new(modern_bpf): add support for `notify` syscalls #516

Andreagit97 · 2022-08-01T08:10:13Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area driver-modern-bpf

/area libpman

/area tests

Does this PR require a change in the driver versions?

What this PR does / why we need it:

This PR is part of a series #513, the final aim is to support the most important syscalls also in the new probe. This PR introduces:

eventfd
eventfd2
inotify_init
inotify_init1
timerfd_create
userfaultfd
signalfd
signalfd4

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(modern_bpf): add support for `notify` syscalls

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

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

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

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

hbrueckner · 2022-08-02T09:28:08Z

test/modern_bpf/test_suites/syscall_enter_suite/eventfd_e.cpp

+
+	uint32_t initval = 3;
+	int flags = 0;
+	int32_t fd = syscall(__NR_eventfd, initval, flags);


The eventfd and eventfd2 syscalls seem to be interesting. Actually based on the man page of eventfd, it states

There are two underlying Linux system calls: eventfd() and the more recent eventfd2(). The former system call does not implement a flags argument. The latter system call implements the flags values described above. The glibc wrapper function will use eventfd2() where it is available.

The Linux kernel source code seems to confirm that here.

So for the eventfd, the flags argument should not be necessary... only for the eventfd2 syscall.

Was the scap event then actually implemented with eventfd2 in mind?

Yeah, that is the same case of inotify_init and inotify_init1, inotify_init doesn't have the flag but we catch it anyway. The solution could be to create 2 separate events as I have already tracked it in #515 🤔

Ok.. will look at them as well... but still I wonder passing flags to the syscall even if the syscall does not use it. And agree, I would rather split those events into 2 distinct ones.

During this refactor we need to understand which params we need because I agree with you that some of these params are not used at all!!

hbrueckner · 2022-08-02T09:29:47Z

test/modern_bpf/test_suites/syscall_enter_suite/eventfd2_e.cpp

+	evt_test->assert_numeric_param(1, (uint64_t)initval);
+
+	/* Parameter 2: flags (type: PT_FLAGS32) */
+	/* Right now we don't catch any flag, so we expect `0` as a second parameter. */


Being tracked here: #515 (pls also mention it there in addition to eventfd)

great catch! Done, thank you!

hbrueckner · 2022-08-02T12:09:54Z

test/modern_bpf/test_suites/syscall_enter_suite/inotify_init_e.cpp

+	/*=============================== ASSERT PARAMETERS  ===========================*/
+
+	/* Parameter 1: flags (type: PT_FLAGS8) */
+	evt_test->assert_numeric_param(1, (uint8_t)flags);


hmm... so if inotify_init(void) does not process the flags, then the BPF program just passes the first argument simply thru? How does it look like for a simply inotify_init() call where the BPF program then reports the (whatever) content the register for the first argument contains? If so, could that be a security issue/information leak?

Not sure if this is an information leak but it could be... In my tests, it always returns 0, but obviously we are not sure about the content, we must fix it!

Not sure if this is an information leak but it could be...

It could be.... and depending on the architecture ELF ABI, argument registers might become call-clobbered/cleared etc. However, being on the safe side is better.

Thanks for flags removal in eventfd which also had the potential for a leak.

hbrueckner · 2022-08-02T12:18:03Z

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

+
+	/* Parameter 2: flags (type: PT_FLAGS8) */
+	/* Like in the old probe we send `0` */
+	ringbuf__store_u8(&ringbuf, 0);


I guess for both, helpers for to map to scap events are also necessary then.

hbrueckner · 2022-08-02T12:25:28Z

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

+
+	/* Parameter 2: flags (type: PT_FLAGS32) */
+	u32 flags = (u32)extract__syscall_argument(regs, 0);
+	ringbuf__store_u32(&ringbuf, flags);


I guess here we need also userfaultfd_flags_to_scap helper. Yet another one for #515

hbrueckner · 2022-08-02T12:31:05Z

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

+	/* Like in the old probe we send `0`. */
+	ringbuf__store_u32(&ringbuf, 0);
+
+	/* Parameter 3: flags (type: PT_FLAGS8) */


Hmm.. int signalfd(int fd, const sigset_t *mask, int flags); ... should the type be of PT_INT32or PT_FLAGS32; also I guess a helper is required too.

Andreagit97 · 2022-08-02T12:43:39Z

@hbrueckner I have added all your comments on the tracking issue, there is a lot of work to do on that :/

hbrueckner · 2022-08-02T12:38:44Z

test/modern_bpf/test_suites/syscall_enter_suite/signalfd4_e.cpp

+	int32_t mock_fd = -1;
+	sigset_t mask = {0};
+	int flags = 7;
+	assert_syscall_state(SYSCALL_FAILURE, "signalfd4", syscall(__NR_signalfd4, mock_fd, &mask, flags));


This also relates to the signalfd BPF program. Looks like there is a similar pattern like described here.

According to the man page, the system call includes an additional argument, size_t sizemask which is transparently handled by the glibc wrapper. For signalfd4 it is the third argument (where flags is passed). I would assume that for calling signalfd and signalfd4 directly, this parameter needs to be added here as well. WDYT?

(I think for now, this is a blind spot because the BPF programs do not catch those arguments)

For reference, syscall definition in the Linux kernel.

This is really a great catch thank you for your accuracy!! 😍

hbrueckner · 2022-08-02T12:55:54Z

Hi @Andreagit97

@hbrueckner I have added all your comments on the tracking issue, there is a lot of work to do on that :/

Thanks for adding them all! and sorry for all the new todos

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

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>

Andreagit97 · 2022-08-02T22:17:05Z

Hi @Andreagit97

@hbrueckner I have added all your comments on the tracking issue, there is a lot of work to do on that :/

Thanks for adding them all! and sorry for all the new todos

I should have addressed all the comments and I have tracked all the problems in our issue #515. @hbrueckner thank you for the review as always! Just one note, to temporarily address the possible information leakage in inotify_init I have removed the flag collection in the PPME_SYSCALL_INOTIFY_INIT_E event. This was quite useless even because we didn't convert the flags in the scap format... To fix this stuff we will introduce 2 different events, one for the inotify_init and the other for inotify_init1

hbrueckner · 2022-08-03T08:22:29Z

Hi @Andreagit97

I should have addressed all the comments and I have tracked all the problems in our issue #515. @hbrueckner thank you for the review as always! Just one note, to temporarily address the possible information leakage in inotify_init I have removed the flag collection in the PPME_SYSCALL_INOTIFY_INIT_E event. This was quite useless even because we didn't convert the flags in the scap format... To fix this stuff we will introduce 2 different events, one for the inotify_init and the other for inotify_init1

Thank you very much! LGTM and I will give them a try on s390 later this week and share test results.

hbrueckner · 2022-08-03T12:52:26Z

Hi @Andreagit97

Thank you very much! LGTM and I will give them a try on s390 later this week and share test results.

Here we go, all tests have passed as well:

[ RUN      ] SyscallExit.eventfd2X
[       OK ] SyscallExit.eventfd2X (0 ms)
[ RUN      ] SyscallExit.eventfdX
[       OK ] SyscallExit.eventfdX (0 ms)
[ RUN      ] SyscallExit.inotify_init1X
[       OK ] SyscallExit.inotify_init1X (0 ms)
[ RUN      ] SyscallExit.inotify_initX
[       OK ] SyscallExit.inotify_initX (0 ms)
[ RUN      ] SyscallExit.signalfd4X
[       OK ] SyscallExit.signalfd4X (0 ms)
[ RUN      ] SyscallExit.signalfdX
[       OK ] SyscallExit.signalfdX (0 ms)
[ RUN      ] SyscallExit.timerfd_createX
[       OK ] SyscallExit.timerfd_createX (0 ms)
[ RUN      ] SyscallExit.userfaultfdX
[       OK ] SyscallExit.userfaultfdX (0 ms)

[ RUN      ] SyscallEnter.eventfd2E
[       OK ] SyscallEnter.eventfd2E (0 ms)
[ RUN      ] SyscallEnter.eventfdE
[       OK ] SyscallEnter.eventfdE (0 ms)
[ RUN      ] SyscallEnter.inotify_init1E
[       OK ] SyscallEnter.inotify_init1E (0 ms)
[ RUN      ] SyscallEnter.inotify_initE
[       OK ] SyscallEnter.inotify_initE (0 ms)
[ RUN      ] SyscallEnter.signalfd4E
[       OK ] SyscallEnter.signalfd4E (0 ms)
[ RUN      ] SyscallEnter.signalfdE
[       OK ] SyscallEnter.signalfdE (0 ms)
[ RUN      ] SyscallEnter.timerfd_createE
[       OK ] SyscallEnter.timerfd_createE (0 ms)
[ RUN      ] SyscallEnter.userfaultfdE
[       OK ] SyscallEnter.userfaultfdE (0 ms)

FedeDP

/approve

poiana · 2022-08-03T13:03:02Z

LGTM label has been added.

Git tree hash: 437013d555e7d31d1aeb1e35c20c07502c8a3803

poiana · 2022-08-03T13:03:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

poiana added release-note kind/feature New feature or request dco-signoff: yes area/driver-modern-bpf area/libpman labels Aug 1, 2022

poiana requested review from gnosek and leogr August 1, 2022 08:10

poiana added area/tests approved size/XXL labels Aug 1, 2022

Andreagit97 commented Aug 1, 2022

View reviewed changes

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/inotify_init.bpf.c Outdated Show resolved Hide resolved

Andreagit97 commented Aug 1, 2022

View reviewed changes

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/eventfd.bpf.c Show resolved Hide resolved

Andreagit97 commented Aug 1, 2022

View reviewed changes

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/signalfd.bpf.c Show resolved Hide resolved

Andreagit97 commented Aug 1, 2022

View reviewed changes

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/timerfd_create.bpf.c Show resolved Hide resolved

hbrueckner reviewed Aug 2, 2022

View reviewed changes

Andreagit97 mentioned this pull request Aug 2, 2022

[Tracking] Params inconsistencies in our drivers #515

Open

59 tasks

hbrueckner reviewed Aug 2, 2022

View reviewed changes

Andreagit97 added 7 commits August 3, 2022 00:06

new(modern_bpf): add eventfd syscall

14c5439

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

new(modern_bpf): add eventfd2 syscall

81f6a0f

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

new(modern_bpf): add inotify_init syscall

04ed4bc

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

new(modern_bpf): add inotify_init1 syscall

aec35cc

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

new(modern_bpf): add timerfd_create syscall

2751f6d

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

new(modern_bpf): add userfaultfd syscall

7ee1085

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

new(modern_bpf): add signalfd syscall

7767b3e

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

Andreagit97 and others added 4 commits August 3, 2022 00:06

new(modern_bpf): add signalfd4 syscall

33a94ad

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

test(modern_bpf): remove useless flags params

ad4a3d9

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>

update(modern_bpf): avoid information leakage in inotify_init

a3f74df

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>

update(modern_bpf): add a missing parameter in signalfd syscalls

59aad71

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>

Andreagit97 force-pushed the notify_syscall branch from 2608086 to 59aad71 Compare August 2, 2022 22:08

FedeDP approved these changes Aug 3, 2022

View reviewed changes

poiana assigned FedeDP Aug 3, 2022

poiana added the lgtm label Aug 3, 2022

leogr approved these changes Aug 4, 2022

View reviewed changes

poiana assigned leogr Aug 4, 2022

poiana merged commit de3dee2 into master Aug 4, 2022

poiana deleted the notify_syscall branch August 4, 2022 11:03

FedeDP added this to the 3.0.0+driver milestone Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(modern_bpf): add support for `notify` syscalls #516

new(modern_bpf): add support for `notify` syscalls #516

Andreagit97 commented Aug 1, 2022

hbrueckner Aug 2, 2022

Andreagit97 Aug 2, 2022

hbrueckner Aug 2, 2022

Andreagit97 Aug 2, 2022

hbrueckner Aug 2, 2022 •

edited

Loading

Andreagit97 Aug 2, 2022

hbrueckner Aug 2, 2022

Andreagit97 Aug 2, 2022

Andreagit97 Aug 2, 2022

hbrueckner Aug 2, 2022

hbrueckner Aug 3, 2022

hbrueckner Aug 2, 2022

hbrueckner Aug 2, 2022

hbrueckner Aug 2, 2022

Andreagit97 commented Aug 2, 2022 •

edited

Loading

hbrueckner Aug 2, 2022

Andreagit97 Aug 2, 2022

hbrueckner commented Aug 2, 2022

Andreagit97 commented Aug 2, 2022

hbrueckner commented Aug 3, 2022

hbrueckner commented Aug 3, 2022

FedeDP left a comment

poiana commented Aug 3, 2022

poiana commented Aug 3, 2022

new(modern_bpf): add support for notify syscalls #516

new(modern_bpf): add support for notify syscalls #516

Conversation

Andreagit97 commented Aug 1, 2022

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 Aug 2, 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

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 Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbrueckner commented Aug 2, 2022

Andreagit97 commented Aug 2, 2022

hbrueckner commented Aug 3, 2022

hbrueckner commented Aug 3, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Aug 3, 2022

poiana commented Aug 3, 2022

new(modern_bpf): add support for `notify` syscalls #516

new(modern_bpf): add support for `notify` syscalls #516

hbrueckner Aug 2, 2022 •

edited

Loading

Andreagit97 commented Aug 2, 2022 •

edited

Loading