new: implement some syscalls in the modern probe (1/n) #968

Andreagit97 · 2023-03-10T16:48:42Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

This PR is part of a series that aims to address 👉 #723
More in detail this PR implements:

brk
getrlimit
ugetrlimit (used only on arm 32 bit)
send (used only on arm 32 bit)
recv (used only on arm 32 bit)

Which issue(s) this PR fixes:

Part of #723

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

Andreagit97 · 2023-03-10T16:50:24Z

driver/bpf/fillers.h

+	/* Parameter 1: res (type: PT_ERRNO) */
+	long retval = bpf_syscall_get_retval(data->ctx);
+	int res = bpf_val_to_ring_type(data, retval, PT_ERRNO);
+	CHECK_RES(res);

-	/*
-	 * data
+	/* Parameter 2: data (type: PT_BYTEBUF) */
+	/* If the syscall doesn't fail we use the return value as `size`
+	 * otherwise we need to rely on the syscall parameter provided by the user.
 	 */
-	if (retval < 0) {
-		/*
-		 * The operation failed, return an empty buffer
-		 */
-		val = 0;
-		bufsize = 0;
-	} else {
-		val = bpf_syscall_get_argument(data, 1);
-
-		/*
-		 * The return value can be lower than the value provided by the user,
-		 * and we take that into account.
-		 */
-		bufsize = retval;
-	}
-
+	unsigned long bytes_to_read = retval > 0 ? retval : bpf_syscall_get_argument(data, 2);
+	unsigned long sent_data_pointer = bpf_syscall_get_argument(data, 1);
 	data->fd = bpf_syscall_get_argument(data, 0);
-	res = __bpf_val_to_ring(data, val, bufsize, PT_BYTEBUF, -1, true, USER);
-
-	return res;
+	return __bpf_val_to_ring(data, sent_data_pointer, bytes_to_read, PT_BYTEBUF, -1, true, USER);


Refactor to follow the new approach outlined here #942

"write-like" syscalls should always send data to userspace even when they fail

Read through #942, makes sense to me, thanks everyone for sharing the details!

Andreagit97 · 2023-03-10T16:51:11Z

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/brk.bpf.c

+	/*=============================== COLLECT PARAMETERS  ===========================*/
+
+	/* Parameter 1: ret (type: PT_UINT64) */
+	/* the return value is the program break */


the return value is not the classic errno, it is the address of program break

Andreagit97 · 2023-03-10T16:51:59Z

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/recv.bpf.c

+	else
+	{
+		/* Parameter 2: data (type: PT_BYTEBUF) */
+		auxmap__store_empty_param(auxmap);
+	}


we are in a "read-like" syscall so we send data only in case of success

Andreagit97 · 2023-03-10T16:52:23Z

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/send.bpf.c

+	unsigned long sent_data_pointer = args[1];
+	auxmap__store_bytebuf_param(auxmap, sent_data_pointer, bytes_to_read, USER);


"write-like" syscall, we always send data!

Andreagit97 · 2023-03-10T16:53:19Z

driver/ppm_fillers.c

-	if (unlikely(res != PPM_SUCCESS))
-		return res;
+	res = val_to_ring(args, sent_data_pointer, bufsize, true, 0);
+	CHECK_RES(res);


Dual of the bpf refactor

@hbrueckner could you double-check on all the socketcall refactor, ty!

@Andreagit97 @incertum reviewed this PR and socketcall changes look good.

Andreagit97 · 2023-03-10T16:56:05Z

test/drivers/test_suites/syscall_exit_suite/recv_x.cpp

+#include "../../event_class/event_class.h"
+
+#ifdef __NR_recv
+TEST(SyscallExit, recvX_fail)


this syscall is never called in our supported architectures, I've added just one test to check if the filler is correctly called. If one day we will support an arch with this syscall we could add further tests.
Same for send syscall

Andreagit97 · 2023-03-10T16:57:30Z

test/drivers/test_suites/syscall_enter_suite/ugetrlimit_e.cpp

+	/* Please note:
+	 * the syscall `ugetrlimit` is mapped to `PPME_SYSCALL_GETRLIMIT_E` event
+	 * like `getrlimit`. The same BPF program will be used for both the syscalls.
+	 */


right now we don't have a dedicated event pair for ugetrlimit so we use the same fillers of getrlimit. This issue is tracked here #911

FedeDP

LGTM!
BTW I honestly think that the drivers test suites is possibly the coolest thing that happened in the last 6 months! Kudos @Andreagit97
That plus the fact that we now enforce that every syscall added must be added with its own tests, it's an incredible step forward.
/approve

poiana · 2023-03-13T09:45:32Z

LGTM label has been added.

Git tree hash: 2cb4d6a91d314858894cd4bb4abefa10c58397e2

hbrueckner

Hi @Andreagit97
excellent work as always! Thanks for taking care on the socketcall integration and respective tests for recv and send.

/approve

hbrueckner · 2023-03-13T16:55:12Z

driver/ppm_fillers.c

-	if (unlikely(res != PPM_SUCCESS))
-		return res;
+	res = val_to_ring(args, sent_data_pointer, bufsize, true, 0);
+	CHECK_RES(res);


@Andreagit97 @incertum reviewed this PR and socketcall changes look good.

poiana · 2023-03-13T16:56:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, hbrueckner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Andreagit97,FedeDP]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Andreagit97 added 5 commits March 10, 2023 09:26

new: support brk syscall

083136f

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

new: support getrlimit syscall

5800621

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

new: support ugetrlimit syscall

111bcf3

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

new: support send syscall

068bfe0

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

new: support recv syscall

add9a61

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

poiana added release-note-none kind/feature New feature or request dco-signoff: yes area/driver-kmod area/driver-bpf area/driver-modern-bpf labels Mar 10, 2023

Andreagit97 added this to the next-driver milestone Mar 10, 2023

poiana added approved size/XXL labels Mar 10, 2023

poiana requested review from hbrueckner and incertum March 10, 2023 16:48

Andreagit97 commented Mar 10, 2023

View reviewed changes

FedeDP approved these changes Mar 13, 2023

View reviewed changes

poiana assigned FedeDP Mar 13, 2023

poiana added the lgtm label Mar 13, 2023

hbrueckner approved these changes Mar 13, 2023

View reviewed changes

poiana assigned hbrueckner Mar 13, 2023

poiana merged commit 746a8af into falcosecurity:master Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new: implement some syscalls in the modern probe (1/n) #968

new: implement some syscalls in the modern probe (1/n) #968

Andreagit97 commented Mar 10, 2023

Andreagit97 Mar 10, 2023 •

edited

Loading

incertum Mar 13, 2023

Andreagit97 Mar 10, 2023

Andreagit97 Mar 10, 2023

Andreagit97 Mar 10, 2023

Andreagit97 Mar 10, 2023

incertum Mar 13, 2023

hbrueckner Mar 13, 2023

Andreagit97 Mar 10, 2023 •

edited

Loading

Andreagit97 Mar 10, 2023

FedeDP left a comment

poiana commented Mar 13, 2023

hbrueckner left a comment

hbrueckner Mar 13, 2023

poiana commented Mar 13, 2023

		unsigned long sent_data_pointer = args[1];
		auxmap__store_bytebuf_param(auxmap, sent_data_pointer, bytes_to_read, USER);

new: implement some syscalls in the modern probe (1/n) #968

new: implement some syscalls in the modern probe (1/n) #968

Conversation

Andreagit97 commented Mar 10, 2023

Andreagit97 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 13, 2023

hbrueckner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiana commented Mar 13, 2023

Andreagit97 Mar 10, 2023 •

edited

Loading

Andreagit97 Mar 10, 2023 •

edited

Loading