-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new(modern_bpf): add support for notify
syscalls
#516
Conversation
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/inotify_init.bpf.c
Outdated
Show resolved
Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/eventfd.bpf.c
Show resolved
Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/signalfd.bpf.c
Show resolved
Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/timerfd_create.bpf.c
Show resolved
Hide resolved
|
||
uint32_t initval = 3; | ||
int flags = 0; | ||
int32_t fd = syscall(__NR_eventfd, initval, flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!!
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. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being tracked here: #515 (pls also mention it there in addition to eventfd
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch! Done, thank you!
/*=============================== ASSERT PARAMETERS ===========================*/ | ||
|
||
/* Parameter 1: flags (type: PT_FLAGS8) */ | ||
evt_test->assert_numeric_param(1, (uint8_t)flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flags removal in eventfd
which also had the potential for a leak.
|
||
/* Parameter 2: flags (type: PT_FLAGS8) */ | ||
/* Like in the old probe we send `0` */ | ||
ringbuf__store_u8(&ringbuf, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for both, helpers for to map to scap events are also necessary then.
|
||
/* Parameter 2: flags (type: PT_FLAGS32) */ | ||
u32 flags = (u32)extract__syscall_argument(regs, 0); | ||
ringbuf__store_u32(&ringbuf, flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess here we need also userfaultfd_flags_to_scap
helper. Yet another one for #515
/* Like in the old probe we send `0`. */ | ||
ringbuf__store_u32(&ringbuf, 0); | ||
|
||
/* Parameter 3: flags (type: PT_FLAGS8) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. int signalfd(int fd, const sigset_t *mask, int flags);
... should the type be of PT_INT32
or PT_FLAGS32
; also I guess a helper is required too.
@hbrueckner I have added all your comments on the tracking issue, there is a lot of work to do on that :/ |
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really a great catch thank you for your accuracy!! 😍
Hi @Andreagit97
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>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
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>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>
2608086
to
59aad71
Compare
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 |
Hi @Andreagit97
Thank you very much! LGTM and I will give them a try on s390 later this week and share test results. |
Hi @Andreagit97
Here we go, all tests have passed as well:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 437013d555e7d31d1aeb1e35c20c07502c8a3803
|
[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:
Approvers can indicate their approval by writing |
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?: