docs: add an already existing syscall into the modern probe #781

Andreagit97 · 2022-12-09T09:59:26Z

What type of PR is this?

/kind documentation

Any specific area of the project related to this PR?

/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 a first attempt to explain how to add an already existing syscall into the modern bpf probe. I think this document is quite detailed but obviously, it cannot cover all the corner cases :/ Let me know what you think about that :)

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

poiana · 2022-12-09T09:59:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97

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:

~~driver/OWNERS~~ [Andreagit97]

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

Andreagit97 · 2022-12-09T10:01:37Z

Sorry, @dwindsor I'm a bit late, in the meanwhile, you have already learned on your skin how to add a syscall 😂 but i hope that this will help other new contributors maybe :)

dwindsor · 2022-12-09T14:48:00Z

Thanks for this! The diagram of scap_evt is helpful. Should we also update this article with a link to these instructions?

Andreagit97 · 2022-12-09T14:58:07Z

Yeah, when we are ready I will do a sort of blog post with this new info, it will be the second part of this one article

the first article explains how to add a new syscall from scratch
the new one will explain how to add the just implemented syscall into the modern bpf framework

hbrueckner

Hi @Andreagit97 very nice and focussed tutorial and it reads fantastic!

hbrueckner · 2022-12-12T17:37:46Z

driver/modern_bpf/add_new_syscalls.md

+  * (https://github.com/falcosecurity/libs/blob/master/driver/modern_bpf/helpers/store/ringbuf_store_params.h#L234-L248)
+  * we need to use the `ringbuf__store_u32` helper.
+  */
+ ringbuf__store_u32(&ringbuf, access_flags_to_scap(mode));


It would be worth to have note on xx_flags_to_scap in sense of abstracting flags for the userspace libs.

sure I will add it, ty!

hbrueckner · 2022-12-12T17:40:59Z

driver/modern_bpf/add_new_syscalls.md

+TEST(SyscallExit, <your_syscall_name>X)
+/**/
+{
+ auto evt_test = get_syscall_event_test(<syscall_code>, <event_direction>);


Wondering to add the #ifdef __NR_xxxx to the template to avoid running tests on architectures where a particular syscall does not exist?.

Also, any plans to ship those templates for copy/paste or ultimatively with some m4 magic 😁 ? Wdyt?

Wondering to add the #ifdef __NR_xxxx to the template to avoid running tests on architectures where a particular syscall does not exist?.

Makes sense

Also, any plans to ship those templates for copy/paste or ultimatively with some m4 magic grin ? Wdyt?

Yeah it could be an idea, I was thinking of a simple cpp file as a template, but with more time we can think also something more complex

Sure.. let's start simple and smart here!!

hbrueckner · 2022-12-12T17:43:47Z

driver/modern_bpf/add_new_syscalls.md

+  * to store a `charbuf` param, so one between `PT_CHARBUF`, `PT_FSPATH`, `PT_FSRELPATH`, we need to use
+  * the `auxmap__store_charbuf_param` helper.
+  * - `MAX_PATH` is the maximum length we want to read from the pathname pointer
+  * - `USER` means that the provided pointer points to userspace memory and not kernel memory.


Let me know if I should compile something on the kernel vs. user space memory here or elsewhere 😄

yes if you want you can explain a little bit better here why we need this distinction :) it would be amazing

Thanks... then let me write something up and share with you early next week (being on a training the rest of this week).

dwindsor · 2022-12-22T21:18:36Z

driver/modern_bpf/add_new_syscalls.md

+ evt_test->assert_num_params_pushed(2);
+}
+```
+


Suggested change

## Handling syscalls with complex parameters

Some syscalls' parameters behave differently in the error case. For instance, consider `read(2)`:

#include <unistd.h>

ssize_t read(int fd, void *buf, size_t count);

`read` behaves differently when an error occurs:

RETURN VALUE

On success, the number of bytes read is returned (zero indicates end of file), and the file position is advanced by this number. It is not

an error if this number is smaller than the number of bytes requested; this may happen for example because fewer bytes are actually available

right now (maybe because we were close to end-of-file, or because we are reading from a pipe, or from a terminal), or because read() was in‐

terrupted by a signal. See also NOTES.

On error, -1 is returned, and errno is set appropriately. In this case, it is left unspecified whether the file position (if any) changes.

Writing a driver for this syscall is a bit trickier than usual. In the success case, the

exit probe should return the number of bytes read as well as the bytes themselves through the `*buf` parameter. In the error case, the exit probe should return `errno` and nothing/empty for `*buf`.

To verify proper driver handling for both error and success cases, two tests should be created:

### Complex parameter probe

SEC("tp_btf/sys_exit")

int BPF_PROG(read_x,

struct pt_regs *regs,

long ret)

{

// ...

/*=============================== COLLECT PARAMETERS ===========================*/

/* Parameter 1: res (type: PT_ERRNO) */

auxmap__store_s64_param(auxmap, ret);

if(ret > 0)

{

/* SUCCESS CASE - push a param containing the read buffer */

/* Parameter 2: data (type: PT_BYTEBUF) */

unsigned long data_pointer = extract__syscall_argument(regs, 1);

auxmap__store_bytebuf_param(auxmap, data_pointer, bytes_to_read, USER);

}

else

{

/* ERROR CASE - push an empty param */

/* Parameter 2: data (type: PT_BYTEBUF) */

auxmap__store_empty_param(auxmap);

}

/*=============================== COLLECT PARAMETERS ===========================*/

// ...

}

### Complex parameter test (Success case)

TEST(SyscallExit, readX)

{

// ...

/*=============================== TRIGGER SYSCALL ===========================*/

/* Open /dev/urandom for reading */

int fd = syscall(__NR_open, "/dev/urandom", O_RDONLY);

assert_syscall_state(SYSCALL_SUCCESS, "open", fd, NOT_EQUAL, -1);

/* Read data from /dev/urandom */

const unsigned data_len = 64;

char buf[data_len];

ssize_t read_bytes = syscall(__NR_read, fd, (void *)buf, data_len);

assert_syscall_state(SYSCALL_SUCCESS, "read", read_bytes, NOT_EQUAL, -1);

/* Close /dev/urandom fd */

assert_syscall_state(SYSCALL_SUCCESS, "close", syscall(__NR_close, fd), NOT_EQUAL, -1);

/*=============================== TRIGGER SYSCALL ===========================*/

// ...

/*=============================== ASSERT PARAMETERS ===========================*/

/* Parameter 1: res (type: PT_ERRNO) */

evt_test->assert_numeric_param(1, (int64_t)read_bytes);

/* Parameter 2: data (type: PT_BYTEBUF) */

evt_test->assert_only_param_len(2, (uint32_t)read_bytes);

/*=============================== ASSERT PARAMETERS ===========================*/

// ...

}

### Complex parameter test (Failure case)

TEST(SyscallExit, readXfailure)

{

// ...

/*=============================== TRIGGER SYSCALL ===========================*/

/* Read data from an invalid fd */

const unsigned data_len = 64;

char buf[data_len];

ssize_t read_bytes = syscall(__NR_read, -1, (void *)buf, data_len);

int errno_value = -errno;

assert_syscall_state(SYSCALL_FAILURE, "read", read_bytes);

/*=============================== TRIGGER SYSCALL ===========================*/

// ...

/*=============================== ASSERT PARAMETERS ===========================*/

/* Parameter 1: res (type: PT_ERRNO) */

evt_test->assert_numeric_param(1, (int64_t)errno_value);

/* Parameter 2: data (type: PT_BYTEBUF) */

evt_test->assert_empty_param(2);

/*=============================== ASSERT PARAMETERS ===========================*/

// ...

}

thank you for the suggestion ;) I will try to create version 2 of this document ASAP :)

Andreagit97 · 2023-03-22T14:18:42Z

I will move the content of this document into a new Blog post or merge it into an existing one falcosecurity/falco-website#766 so we can close this :)

docs: add an already existing syscall into the modern probe

aa6b358

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

poiana added kind/documentation Improvements or additions to documentation release-note-none dco-signoff: yes area/driver-modern-bpf labels Dec 9, 2022

poiana requested review from gnosek and hbrueckner December 9, 2022 09:59

poiana added approved size/XXL labels Dec 9, 2022

Andreagit97 modified the milestones: 0.11.0, next-driver Dec 9, 2022

hbrueckner reviewed Dec 12, 2022

View reviewed changes

dwindsor reviewed Dec 22, 2022

View reviewed changes

dwindsor mentioned this pull request Mar 15, 2023

Support Linux Security Modules hook for more accurate security tracing #252

Open

23 tasks

Andreagit97 closed this Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add an already existing syscall into the modern probe #781

docs: add an already existing syscall into the modern probe #781

Andreagit97 commented Dec 9, 2022

poiana commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

dwindsor commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

hbrueckner left a comment

hbrueckner Dec 12, 2022

Andreagit97 Dec 13, 2022

hbrueckner Dec 12, 2022

Andreagit97 Dec 13, 2022

hbrueckner Dec 13, 2022

hbrueckner Dec 12, 2022 •

edited

Loading

Andreagit97 Dec 13, 2022

hbrueckner Dec 13, 2022

dwindsor Dec 22, 2022

Andreagit97 Dec 23, 2022

Andreagit97 commented Mar 22, 2023

+## Handling syscalls with complex parameters
+Some syscalls' parameters behave differently in the error case. For instance, consider `read(2)`:
+#include <unistd.h>
+ssize_t read(int fd, void *buf, size_t count);
+`read` behaves differently when an error occurs:
+RETURN VALUE
+       On  success,  the number of bytes read is returned (zero indicates end of file), and the file position is advanced by this number.  It is not
+       an error if this number is smaller than the number of bytes requested; this may happen for example because fewer bytes are actually available
+       right  now (maybe because we were close to end-of-file, or because we are reading from a pipe, or from a terminal), or because read() was in‐
+       terrupted by a signal.  See also NOTES.
+       On error, -1 is returned, and errno is set appropriately.  In this case, it is left unspecified whether the file position (if any) changes.
+Writing a driver for this syscall is a bit trickier than usual. In the success case, the
+exit probe should return the number of bytes read as well as the bytes themselves through the `*buf` parameter. In the error case, the exit probe should return `errno` and nothing/empty for `*buf`.
+To verify proper driver handling for both error and success cases, two tests should be created:
+### Complex parameter probe
+SEC("tp_btf/sys_exit")
+int BPF_PROG(read_x,
+             struct pt_regs *regs,
+             long ret)
+{
+// ...
+/*=============================== COLLECT PARAMETERS  ===========================*/
+/* Parameter 1: res (type: PT_ERRNO) */
+auxmap__store_s64_param(auxmap, ret);
+if(ret > 0)
+{
+	/* SUCCESS CASE - push a param containing the read buffer */
+	/* Parameter 2: data (type: PT_BYTEBUF) */
+        unsigned long data_pointer = extract__syscall_argument(regs, 1);
+        auxmap__store_bytebuf_param(auxmap, data_pointer, bytes_to_read, USER);
+}
+else
+{
+	/* ERROR CASE - push an empty param */
+        /* Parameter 2: data (type: PT_BYTEBUF) */
+        auxmap__store_empty_param(auxmap);
+}
+/*=============================== COLLECT PARAMETERS  ===========================*/
+// ...
+}
+### Complex parameter test (Success case)
+TEST(SyscallExit, readX)
+{
+// ...
+/*=============================== TRIGGER SYSCALL  ===========================*/
+/* Open /dev/urandom for reading */
+int fd = syscall(__NR_open, "/dev/urandom", O_RDONLY);
+assert_syscall_state(SYSCALL_SUCCESS, "open", fd, NOT_EQUAL, -1);
+/* Read data from /dev/urandom */
+const unsigned data_len = 64;
+char buf[data_len];
+ssize_t read_bytes = syscall(__NR_read, fd, (void *)buf, data_len);
+assert_syscall_state(SYSCALL_SUCCESS, "read", read_bytes, NOT_EQUAL, -1);
+/* Close /dev/urandom fd */
+assert_syscall_state(SYSCALL_SUCCESS, "close", syscall(__NR_close, fd), NOT_EQUAL, -1);
+/*=============================== TRIGGER SYSCALL ===========================*/
+// ...
+/*=============================== ASSERT PARAMETERS  ===========================*/
+/* Parameter 1: res (type: PT_ERRNO) */
+evt_test->assert_numeric_param(1, (int64_t)read_bytes);
+/* Parameter 2: data (type: PT_BYTEBUF) */
+evt_test->assert_only_param_len(2, (uint32_t)read_bytes);
+/*=============================== ASSERT PARAMETERS  ===========================*/
+// ...
+}
+### Complex parameter test (Failure case)
+TEST(SyscallExit, readXfailure)
+{
+// ...
+/*=============================== TRIGGER SYSCALL  ===========================*/
+/* Read data from an invalid fd */
+const unsigned data_len = 64;
+char buf[data_len];
+ssize_t read_bytes = syscall(__NR_read, -1, (void *)buf, data_len);
+int errno_value = -errno;
+assert_syscall_state(SYSCALL_FAILURE, "read", read_bytes);
+/*=============================== TRIGGER SYSCALL ===========================*/
+// ...
+/*=============================== ASSERT PARAMETERS  ===========================*/
+/* Parameter 1: res (type: PT_ERRNO) */
+evt_test->assert_numeric_param(1, (int64_t)errno_value);
+/* Parameter 2: data (type: PT_BYTEBUF) */
+evt_test->assert_empty_param(2);
+/*=============================== ASSERT PARAMETERS  ===========================*/
+// ...
+}

docs: add an already existing syscall into the modern probe #781

docs: add an already existing syscall into the modern probe #781

Conversation

Andreagit97 commented Dec 9, 2022

poiana commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

dwindsor commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

hbrueckner left a 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

hbrueckner Dec 12, 2022 • 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

Andreagit97 commented Mar 22, 2023

hbrueckner Dec 12, 2022 •

edited

Loading