new(bpf, modern_bpf): introduce `socketcall` handling #811

hbrueckner · 2022-12-21T14:10:02Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area driver-bpf

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Add socketcall handling to enable capturing of network syscall events on s390x. See #810 for details.

Which issue(s) this PR fixes:

Fixes the BPF part in: #810
Depends-on: #809

Special notes for your reviewer:

I took an approach similar to the kmod driver to extract the socket call and its arguments. Arguments are stored and looked up in/from a BPF map. This becomes then transparent to the actual filler implementations.

Note that this PR includes and is depending on #809, so for review please focus on the just the last commit.

cc: @FedeDP @Andreagit97 @Molter73

@Andreagit97 The modern-bpf version of socketcall handling is not yet finalized and like to briefly discuss my approach in #810.

Does this PR introduce a user-facing change?:

This should be transparent.

`new(bpf, modern_bpf): introduce `socketcall` handling`

FedeDP · 2022-12-21T14:21:58Z

Since this depends on #809 that is wip, can we make wip this too?

FedeDP · 2022-12-21T14:23:10Z

Oh i see, this is a PR on top of #809. If you set the target branch to be the one used by #809 we are able to review only the correct diff for this patch (instead of the whole diff from master!)

hbrueckner · 2022-12-21T14:55:37Z

Oh i see, this is a PR on top of #809. If you set the target branch to be the one used by #809 we are able to review only the correct diff for this patch (instead of the whole diff from master!)

I guess, I need to push #809 as branch into libs directly...? I just have them external and I guess an external target branch will not work.

FedeDP · 2022-12-21T14:58:08Z

You are probably right; well let's keep this as is then!

Andreagit97 · 2022-12-21T16:52:55Z

I will have a deeper look at it but it seems great! I think we can do something similar for the modern bpf probe 🤔

hbrueckner · 2023-02-07T08:44:28Z

Hi @FedeDP , @Andreagit97 thanks for merging #809! So now here is the rebased version of the socketcall support in the BPF.

FedeDP · 2023-02-07T08:47:32Z

Hi @hbrueckner ! Does this PR fix support of old bpf probe for s390x?
If affirmative, can you also update the README driver support table? ;)
Thank you!

hbrueckner · 2023-02-07T13:03:15Z

Hi @hbrueckner ! Does this PR fix support of old bpf probe for s390x? If affirmative, can you also update the README driver support table? ;) Thank you!

Yeah.. will do! Additional commit comes later today...

Andreagit97 · 2023-02-07T13:40:21Z

driver/bpf/maps.h

@@ -99,6 +99,15 @@ struct bpf_map_def __bpf_section("maps") stash_map = {
 };
 #endif

+#ifdef CAPTURE_SOCKETCALL
+struct bpf_map_def __bpf_section("maps") socketcall_args_map = {


you need to add also an enum for this map https://github.com/falcosecurity/libs/blob/master/driver/bpf/types.h#L216-L230, otherwise, the userspace could parse them out of order. something like SCAP_SOCKETCALL_MAP should be ok

Thanks @Andreagit97 . Should be solved with this commit.

hbrueckner · 2023-02-07T16:36:36Z

Hi @Andreagit97, @FedeDP ,

need some hints from you regarding some initial socketcall testing. I started to modify the socket and bind driver test cases to have a direct and socketcall-based version (with mininal changes as usual ;-).

What I actually realized with initial test failures is that socketcall needs to be marked as "interesting" to create the desired events. In the current implementation the check for enabled = is_syscall_interesting(id); is quite early and is before the handle_socketcall logic is triggered.

The question is how to best handle this:

For the driver tests, I can imagine a specific solution in the event_class...
In general
a) should the userspace add socketcall to be interesting when network calls are involved?
b) should is_syscall_interesting be extended to cover socketcall and go even further with sub-filtering on the respective socket calls (based that s390xhas direct syscall support since Linux kernel v4.3 and for old BPF driver v5.5 is required anyway)

Wdyt?

For modern BPF, this is quite similar and I already have a prototype which goes into direction b). And, ultimatively, I guess @incertum might be interesting here as well to install very specific system calls only.

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

hbrueckner · 2023-03-01T15:43:44Z

Hi @Andreagit97 , @FedeDP

it took a while to complete this PR with @Andreagit97 suggested changes. Now the socketcall support should handle all cases along with respective tests (passing on kmod, bpf, and modern_bpf).

From a testing perspective, currently those are missing (and hopefully will be added with the respective modern bpf probes):

getsockname
getpeername
send
recv

The PR is very large due to copying the existing network related tests. The most interesting part of the review is the way to use socketcall and to properly set up the arguments. There are also changes to the modern bpf programs to correctly fetch the arguments. This of course, needs to be considered when providing above probe implementations. Note that there was also a bug in the kmod socketcall implementation regarding getsockopt and setsockopt (thanks to our tests!).

Also some special handling was required for accept which is not available on s390x and its mapping to accept4 in the test cases. Because tests are "syscall" based, I had to exclude for kmod with its event-based implementation (requiring some more changes in the event_class).

Take your time to review (better on commit base) ... I will on a conference next week and will be slow in response then.

Andreagit97 · 2023-03-02T16:58:18Z

that's a huge work @hbrueckner thank you very much for that! I will take a look ASAP!

FedeDP · 2023-03-06T08:44:13Z

driver/main.c

@@ -1767,7 +1767,7 @@ static int record_event_consumer(struct ppm_consumer_t *consumer,
 	}

 	// Check if syscall is interesting for the consumer
-	if (event_datap->category == PPMC_SYSCALL)
+	if (event_datap->category == PPMC_SYSCALL && event_datap->event_info.syscall_data.id != event_datap->socketcall_syscall)


Neat fix, thank you!

FedeDP

All in all, this LGTM! Thank you very much for this huge effort!

Note, most of the lines changes are from test suites ;)
I'll wait for @Andreagit97 for the modern probe part!

FedeDP

/approve

poiana · 2023-03-08T10:05:48Z

LGTM label has been added.

Git tree hash: dc60a823fc377ab93999d2d4d4851e6d021218eb

Andreagit97

the unique issue is the __NR_ACCEPT macro that should be converted to __NR_accept, but since the entity of the PR and the conflict risk I prefer to merge it and I will address these minor comments in another one :)
BTW Amazing work, thank you very much for that!
/approve

driver/bpf/plumbing_helpers.h

Andreagit97 · 2023-03-08T10:37:49Z

driver/bpf/plumbing_helpers.h

+	case SYS_ACCEPT:
+#if defined(CONFIG_S390) && defined(__NR_accept4)
+		return __NR_accept4;
+#elif defined(__NR_ACCEPT)


Suggested change

#elif defined(__NR_ACCEPT)

#elif defined(__NR_accept)

Andreagit97 · 2023-03-08T10:45:48Z

driver/modern_bpf/helpers/interfaces/syscalls_dispatcher.h

+	case SYS_ACCEPT:
+#if defined(__TARGET_ARCH_s390) && defined(__NR_accept4)
+		return __NR_accept4;
+#elif defined(__NR_ACCEPT)


Andreagit97 · 2023-03-08T10:58:58Z

driver/ppm_fillers.c

+		syscall_get_arguments_deprecated(current, args->regs, 0, 5, val);
+#ifndef UDIG
+	else
+		memcpy(val, args->socketcall_args, 5*sizeof(syscall_arg_t));


syscall_arg_t is correct but since args->socketcall_args is a vector of unsigned long and not of syscall_arg_t, I would be as explicit as possible

Suggested change

memcpy(val, args->socketcall_args, 5*sizeof(syscall_arg_t));

memcpy(val, args->socketcall_args, 5*sizeof(unsigned long));

Andreagit97 · 2023-03-08T10:59:17Z

driver/ppm_fillers.c

+		syscall_get_arguments_deprecated(current, args->regs, 0, 5, val);
+#ifndef UDIG
+	else
+		memcpy(val, args->socketcall_args, 5*sizeof(syscall_arg_t));


Same here

Suggested change

memcpy(val, args->socketcall_args, 5*sizeof(syscall_arg_t));

memcpy(val, args->socketcall_args, 5*sizeof(unsigned long));

poiana · 2023-03-08T11:09:59Z

[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

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

hbrueckner · 2023-03-08T14:31:38Z

Hi @Andreagit97 ,

the unique issue is the __NR_ACCEPT macro that should be converted to __NR_accept, but since the entity of the PR and the conflict risk I prefer to merge it and I will address these minor comments in another one :) BTW Amazing work, thank you very much for that!

Thanks for spotting the __NR_ACCEPT and the other things! Glad to see this now being merged! ❤️

poiana added release-note kind/feature New feature or request area/driver-bpf dco-signoff: yes labels Dec 21, 2022

poiana requested review from gnosek and leogr December 21, 2022 14:10

poiana added approved size/XXL labels Dec 21, 2022

hbrueckner changed the title ~~new(bpf): introduce socketcall handling~~ wip: new(bpf): introduce socketcall handling Dec 21, 2022

poiana added the do-not-merge/work-in-progress label Dec 21, 2022

Andreagit97 added this to the next-driver milestone Dec 22, 2022

hbrueckner force-pushed the socketcall-bpf-driver-2022-12-21 branch from af390de to 468f52f Compare February 7, 2023 08:43

poiana removed the size/XXL label Feb 7, 2023

hbrueckner changed the title ~~wip: new(bpf): introduce socketcall handling~~ new(bpf): introduce socketcall handling Feb 7, 2023

poiana added size/L and removed do-not-merge/work-in-progress labels Feb 7, 2023

Andreagit97 reviewed Feb 7, 2023

View reviewed changes

hbrueckner force-pushed the socketcall-bpf-driver-2022-12-21 branch from 468f52f to 2217dd4 Compare February 7, 2023 17:22

poiana added size/XXL and removed approved size/L labels Feb 7, 2023

hbrueckner added 7 commits March 1, 2023 16:20

new(tests): socketcall tests for sendto

a0c1929

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

new(tests): socketcall tests for sendmsg

8579f52

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

new(tests): socketcall tests for recvmsg

53d1fbe

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

new(tests): socketcall tests for getsockopt

b6cf4a4

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

new(tests): socketcall tests for setsockopt

81f27be

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

update(tests): enclose all socketcall tests with __NR_socketcall

0ab2f22

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

fix(kmod): use socketcall args in {get,set}sockopt_x

d63aa75

Signed-off-by: Hendrik Brueckner <brueckner@de.ibm.com>

hbrueckner changed the title ~~new(bpf): introduce socketcall handling~~ new(bpf, modern_bpf): introduce socketcall handling Mar 1, 2023

hbrueckner force-pushed the socketcall-bpf-driver-2022-12-21 branch from 2217dd4 to d63aa75 Compare March 1, 2023 15:36

hbrueckner mentioned this pull request Mar 3, 2023

new(modern_bpf): add readv,preadv,pread syscall #917

Merged

FedeDP reviewed Mar 6, 2023

View reviewed changes

Andreagit97 mentioned this pull request Mar 7, 2023

[FEATURE/BUG] Verifier issues with the current bpf probe #940

Closed

FedeDP approved these changes Mar 8, 2023

View reviewed changes

poiana assigned FedeDP Mar 8, 2023

poiana added the lgtm label Mar 8, 2023

poiana added the approved label Mar 8, 2023

Andreagit97 approved these changes Mar 8, 2023

View reviewed changes

poiana assigned Andreagit97 Mar 8, 2023

poiana merged commit 1720439 into falcosecurity:master Mar 8, 2023

Andreagit97 added a commit to Andreagit97/libs that referenced this pull request Mar 8, 2023

fix(driver): address some minor comments relative to falcosecurity#811

55cb2d8

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

Andreagit97 mentioned this pull request Mar 8, 2023

fix(driver): address some minor comments relative to #811 #949

Merged

poiana pushed a commit that referenced this pull request Mar 8, 2023

fix(driver): address some minor comments relative to #811

3f4bbdb

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

hbrueckner deleted the socketcall-bpf-driver-2022-12-21 branch March 8, 2023 14:30

hbrueckner mentioned this pull request Apr 13, 2023

new(bpf, modern-bpf): Add socketcall support #810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(bpf, modern_bpf): introduce `socketcall` handling #811

new(bpf, modern_bpf): introduce `socketcall` handling #811

hbrueckner commented Dec 21, 2022 •

edited

Loading

FedeDP commented Dec 21, 2022

FedeDP commented Dec 21, 2022

hbrueckner commented Dec 21, 2022

FedeDP commented Dec 21, 2022

Andreagit97 commented Dec 21, 2022

hbrueckner commented Feb 7, 2023

FedeDP commented Feb 7, 2023

hbrueckner commented Feb 7, 2023

Andreagit97 Feb 7, 2023

hbrueckner Feb 7, 2023

hbrueckner commented Feb 7, 2023

hbrueckner commented Mar 1, 2023 •

edited

Loading

Andreagit97 commented Mar 2, 2023

FedeDP Mar 6, 2023

FedeDP left a comment

FedeDP left a comment

poiana commented Mar 8, 2023

Andreagit97 left a comment

Andreagit97 Mar 8, 2023

Andreagit97 Mar 8, 2023

Andreagit97 Mar 8, 2023

Andreagit97 Mar 8, 2023

poiana commented Mar 8, 2023

hbrueckner commented Mar 8, 2023

	memcpy(val, args->socketcall_args, 5*sizeof(syscall_arg_t));
	memcpy(val, args->socketcall_args, 5*sizeof(unsigned long));

new(bpf, modern_bpf): introduce socketcall handling #811

new(bpf, modern_bpf): introduce socketcall handling #811

Conversation

hbrueckner commented Dec 21, 2022 • edited Loading

FedeDP commented Dec 21, 2022

FedeDP commented Dec 21, 2022

hbrueckner commented Dec 21, 2022

FedeDP commented Dec 21, 2022

Andreagit97 commented Dec 21, 2022

hbrueckner commented Feb 7, 2023

FedeDP commented Feb 7, 2023

hbrueckner commented Feb 7, 2023

Andreagit97 Feb 7, 2023

Choose a reason for hiding this comment

hbrueckner Feb 7, 2023

Choose a reason for hiding this comment

hbrueckner commented Feb 7, 2023

hbrueckner commented Mar 1, 2023 • edited Loading

Andreagit97 commented Mar 2, 2023

FedeDP Mar 6, 2023

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 8, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

Andreagit97 Mar 8, 2023

Choose a reason for hiding this comment

Andreagit97 Mar 8, 2023

Choose a reason for hiding this comment

Andreagit97 Mar 8, 2023

Choose a reason for hiding this comment

Andreagit97 Mar 8, 2023

Choose a reason for hiding this comment

poiana commented Mar 8, 2023

hbrueckner commented Mar 8, 2023

new(bpf, modern_bpf): introduce `socketcall` handling #811

new(bpf, modern_bpf): introduce `socketcall` handling #811

hbrueckner commented Dec 21, 2022 •

edited

Loading

hbrueckner commented Mar 1, 2023 •

edited

Loading