new(modern_bpf): add support for `open` family syscalls #503

Andreagit97 · 2022-07-26T14:00:09Z

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:

open
openat
openat2
open_by_handle_at

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 `open` family syscalls

Andreagit97 · 2022-07-26T14:05:06Z

test/modern_bpf/test_suites/syscall_exit_suite/openat2_x.cpp

+	evt_test->assert_numeric_param(2, (int64_t)PPM_AT_FDCWD);
+
+	/* Parameter 3: name (type: PT_FSPATH) */
+	evt_test->assert_charbuf_param(3, pathname);
+
+	/* Parameter 4: flags (type: PT_FLAGS32) */
+	evt_test->assert_numeric_param(4, (uint32_t)PPM_O_RDWR | PPM_O_TMPFILE | PPM_O_DIRECTORY);
+
+	/* Parameter 5: mode (type: PT_UINT32) */
+	evt_test->assert_numeric_param(5, (uint32_t)how.mode);
+
+	/* Parameter 6: resolve (type: PT_FLAGS32) */
+	evt_test->assert_numeric_param(6, (uint32_t)PPM_RESOLVE_BENEATH | PPM_RESOLVE_NO_MAGICLINKS);


Still asking myself if it makes sense to assert these parameters also in the failure case... On one side we ensure that also in case of failure we have all the correct parameters, but right now the bpf code is shared apart from the return value which is obviously different in case of failure. It is quite redundant but it could be useful if we change something on the bpf side 🤔 WDYT? (Same for other syscalls where we have more than one test)

I think it is a great thing to test also failure cases, therefore big 👍 from me (even if it will increase verbosity...)

Andreagit97 · 2022-07-26T14:05:08Z

test/modern_bpf/test_suites/syscall_exit_suite/openat2_x.cpp

+
+	/*=============================== ASSERT PARAMETERS  ===========================*/
+
+	evt_test->assert_num_params_pushed(6);


Also here, this could be checked just in one test, it is not necessary to have it in all tests for this event... WDYT?

If the tests can be self-sufficient, all the better IMHO.

driver/modern_bpf/helpers/extract/extract_from_kernel.h

driver/modern_bpf/definitions/missing_definitions.h

test/modern_bpf/test_suites/syscall_exit_suite/open_by_handle_at_x.cpp

poiana · 2022-07-27T13:04:55Z

@hbrueckner: changing LGTM is restricted to collaborators

In response to this:

Hi @Andreagit97 , excellent work!

Just tried c4f28b854 and all tests on s390 passed (running w/o mount-point :)

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gnosek

Nice 👍 Some minor nits only

driver/modern_bpf/helpers/extract/extract_from_kernel.h

driver/modern_bpf/helpers/store/auxmap_store_params.h

gnosek · 2022-07-27T13:51:31Z

test/modern_bpf/test_suites/syscall_exit_suite/openat2_x.cpp

+
+	/*=============================== ASSERT PARAMETERS  ===========================*/
+
+	evt_test->assert_num_params_pushed(6);


If the tests can be self-sufficient, all the better IMHO.

Andreagit97 · 2022-07-27T17:20:37Z

@FedeDP @hbrueckner sorry if bother you again but maybe I found a solution for the mount points, I took inspiration from the original kernel function obviously we cannot reproduce it in the same way :( Let me know if it looks good for you! I've tested it on ARM64 and x86 and all work smooth :)

hbrueckner · 2022-07-28T07:50:38Z

Hi @Andreagit97

I've tested it on ARM64 and x86 and all work smooth :)

Will try it this afternoon on s390 and will review you patches as well... ttyl then

Here are my test results on s390x:

# (cd /tmp/ && /root/git/falcosecurity/libs/build/test/modern_bpf/bpf_test)
[...]
[----------] Global test environment tear-down
[==========] 14 tests from 2 test suites ran. (0 ms total)
[  PASSED  ] 14 tests.

Running directly in /tmp worked fine. Running it locally it failed as well as in this scenario:

# mkdir /tmp/failing-test
(cd /tmp/failing-test && /root/git/falcosecurity/libs/build/test/modern_bpf/bpf_test)
[...]
[ RUN      ] SyscallExit.open_by_handle_atX_success
/root/git/falcosecurity/libs/test/modern_bpf/event_class/event_class.cpp:265: Failure
Expected equality of these values:
  size
    Which is: 44
  expected_size
    Which is: 18
>>>>> length of the param is not correct. Param id = 4

/root/git/falcosecurity/libs/test/modern_bpf/event_class/event_class.cpp:240: Failure
Expected equality of these values:
  m_event_params[m_current_param].valptr
    Which is: "///////////////////////////////failing-test"
  param
    Which is: "/tmp/failing-test"
>>>>> value of the param is not correct. Param id = 4

[  FAILED  ] SyscallExit.open_by_handle_atX_success (0 ms)

Some good news are that I could isolate this issue by creating an additional test that creates a temporary mount point in /tmp:

[ RUN      ] SyscallExit.open_by_handle_atX_success_mp
/root/git/falcosecurity/libs/test/modern_bpf/event_class/event_class.cpp:265: Failure
Expected equality of these values:
  size
    Which is: 79
  expected_size
    Which is: 53
>>>>> length of the param is not correct. Param id = 4

/root/git/falcosecurity/libs/test/modern_bpf/event_class/event_class.cpp:240: Failure
Expected equality of these values:
  m_event_params[m_current_param].valptr
    Which is: "///////////////////////////////modern.bpf.open_by_handle_atX_success_mp.EDysfg"
  param
    Which is: "/tmp/modern.bpf.open_by_handle_atX_success_mp.EDysfg"
>>>>> value of the param is not correct. Param id = 4

[  FAILED  ] SyscallExit.open_by_handle_atX_success_mp (2 ms)

See update(modern_bpf): add open_by_handle_at mount point test and feel free to try it out or cherry-pick.

jasondellaluce

This is fantastic Andrea!

FedeDP

/approve

poiana · 2022-07-28T10:11:44Z

LGTM label has been added.

Git tree hash: 6f8f0331e57bb342b41e3eaa70b8f13a75b6fc7a

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

…ponent 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>

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

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

`__NR_open` is not defined in ARM64, replace it with `__NR_openat` Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>

FedeDP · 2022-07-29T13:09:05Z

Agree!

hbrueckner · 2022-07-29T13:09:24Z

Hi @Andreagit97 ,

I have tried with 12 which seems to have exactly 1 insn over the limit: BPF program is too large. Processed 1000001 insn processed 1000001 insns (limit 1000000) max_states_per_insn 17 total_states 36063 peak_states 185 mark_rea

Same here, 11 is ok, but to be honest I wouldn't push it to the limit, if we change a line there is the risk to break everything :/

Make sense.

Yeah, @hbrueckner I left a comment in the PR for the bpf_d_path helper (https://github.com/falcosecurity/libs/pull/503/files#diff-23e1e00c90562960e7e4110fcc5c62cd89d2ea8a8df3198d7c04512084c7c378R292-R299) and yes we can ask why this helper has been limited to this particular set of kernel functions :/

Thanks a lot!
@iii-i Could you help with the related kernel changes for bpf_d_path()?

@gnosek the problem is that we need also to change the bpf code according to the architecture, change the #define is not enough... maybe we can use a different version of the helper for the specific architecture but I would do it in another PR not here since this one is blocking all the following work.

That's true.. I have (not yet tested) version of the "workaround commit" excluding s390x: f30d50ed703

Just to summarize I would keep this version with max path components 8, I would add a comment to explain the choice, and going on we can think of something better to address it without blocking other PRs. Do you agree with me @FedeDP @hbrueckner @gnosek?

Agreed! And I think it is best to track it separately not holding back your pending other PRs. Thanks a lot!

gnosek · 2022-07-29T13:12:13Z

I'll admit I don't understand why we need different code for s390x, so I just defer to the smarter people in the room ;)

use a final version of the `auxmap__store_path_from_fd` that correctly works on all supported architectures Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com> Co-authored-by: Federico Di Pierro <nierro92@gmail.com>

Andreagit97 · 2022-07-29T13:28:16Z

I left all the commits just to keep the history of our choices and I've done what we agreed! Now we should be ready 🚀

Don't worry @gnosek i will open another PR/issue to explain better what is going on here and how we could solve it :)

FedeDP

/approve

poiana · 2022-07-29T13:30:29Z

LGTM label has been added.

Git tree hash: 076dc95a6e41f9639ddb387283eebaa02afbdcef

poiana · 2022-07-29T13:30:32Z

[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

iii-i · 2022-07-29T13:52:01Z

Here is a relevant thread where people discuss the first attempt to upstream a d_path() BPF helper: https://lore.kernel.org/bpf/cover.1574162990.git.ethercflow@gmail.com/

Apparently d_path() takes a bunch of locks and we need to be sure calling it doesn't cause deadlocks. Furthermore, calling it from an interrupt context will produce nonsensical results (since it accesses current). Finally, there are some TOCTOU concerns when it's applied for security checks.

hbrueckner · 2022-11-16T14:52:16Z

Hi @Andreagit97 , @FedeDP

Also, the CI job build-and-test-modern-bpf-x86 is successful, uhm, really strange thinking It would be amazing to have a CI job also for ARM64 and s390x, but we need external runners for doing this since GH doesn't support these architectures :/

Quick update on the some progress for self-hosted runner support on s390x. @uweigand just opened an issue and draft PR to start the discussion: actions/runner#2263

FedeDP · 2022-11-16T15:02:48Z

This is crazy! Thank you very much for your effort :)
Thanks @uweigand too!

Andreagit97 · 2022-11-16T15:14:57Z

Thank you very much, guys!!

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

poiana requested review from FedeDP and gnosek July 26, 2022 14:00

poiana added area/tests approved size/XXL labels Jul 26, 2022

Andreagit97 commented Jul 26, 2022

View reviewed changes

hbrueckner reviewed Jul 26, 2022

View reviewed changes

driver/modern_bpf/helpers/extract/extract_from_kernel.h Outdated Show resolved Hide resolved

hbrueckner reviewed Jul 26, 2022

View reviewed changes

driver/modern_bpf/definitions/missing_definitions.h Outdated Show resolved Hide resolved

hbrueckner reviewed Jul 26, 2022

View reviewed changes

test/modern_bpf/test_suites/syscall_exit_suite/open_by_handle_at_x.cpp Outdated Show resolved Hide resolved

Andreagit97 force-pushed the open_syscalls branch 3 times, most recently from 5576734 to c4f28b8 Compare July 27, 2022 09:52

gnosek reviewed Jul 27, 2022

View reviewed changes

jasondellaluce reviewed Jul 28, 2022

View reviewed changes

FedeDP previously approved these changes Jul 28, 2022

View reviewed changes

poiana assigned FedeDP Jul 28, 2022

poiana added the lgtm label Jul 28, 2022

FedeDP mentioned this pull request Jul 28, 2022

new(modern_bpf): add support for chdir family syscalls #509

Merged

Andreagit97 dismissed FedeDP’s stale review via 906b52a July 28, 2022 14:49

Andreagit97 force-pushed the open_syscalls branch from 3afc138 to 906b52a Compare July 28, 2022 14:49

poiana removed the lgtm label Jul 28, 2022

Andreagit97 and others added 12 commits July 29, 2022 15:00

cleanup(modern_bpf): clean some code paths

6d4670d

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

new(modern_bpf): add open_by_handle_at syscall

fbb7ed7

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

update(test/modern_bpf): manage test fatal failures

2100b9b

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

fix(modern_bpf): retrieve the right param if there is only 1 path com…

2935a5f

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

update(modern_bpf): simplify a open_by_handle_at test

3d4c099

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

update(modern_bpf): enable the kernel/user read logic

7f8a489

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

update(modern_bpf): flag definitions for s390x

4a7b42a

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

update(modern_bpf): working version supporting mount points

1cdfb88

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

update(modern_bpf): workaround to manage mount points

a66a720

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

fix(modern_bpf): some minor fixes

540a68a

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

update(modern_bpf): add open_by_handle_at mount point test

f234fb6

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

update(test/modern_bpf): fix the ARM64 build

9d7458d

`__NR_open` is not defined in ARM64, replace it with `__NR_openat` Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Hendrik Brueckner <brueckner@de.ibm.com>

Andreagit97 force-pushed the open_syscalls branch from bee4e7b to 4296c71 Compare July 29, 2022 13:24

FedeDP approved these changes Jul 29, 2022

View reviewed changes

poiana added the lgtm label Jul 29, 2022

leogr approved these changes Jul 29, 2022

View reviewed changes

poiana assigned leogr Jul 29, 2022

poiana merged commit 1650e47 into falcosecurity:master Jul 29, 2022

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 `open` family syscalls #503

new(modern_bpf): add support for `open` family syscalls #503

Andreagit97 commented Jul 26, 2022 •

edited

Loading

Andreagit97 Jul 26, 2022

FedeDP Jul 26, 2022

Andreagit97 Jul 26, 2022

gnosek Jul 27, 2022

poiana commented Jul 27, 2022

gnosek left a comment

gnosek Jul 27, 2022

Andreagit97 commented Jul 27, 2022 •

edited

Loading

hbrueckner commented Jul 28, 2022 •

edited

Loading

jasondellaluce left a comment

FedeDP left a comment

poiana commented Jul 28, 2022

FedeDP commented Jul 29, 2022

hbrueckner commented Jul 29, 2022

gnosek commented Jul 29, 2022

Andreagit97 commented Jul 29, 2022

FedeDP left a comment

poiana commented Jul 29, 2022

poiana commented Jul 29, 2022

iii-i commented Jul 29, 2022

hbrueckner commented Nov 16, 2022

FedeDP commented Nov 16, 2022

Andreagit97 commented Nov 16, 2022


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

		evt_test->assert_num_params_pushed(6);

new(modern_bpf): add support for open family syscalls #503

new(modern_bpf): add support for open family syscalls #503

Conversation

Andreagit97 commented Jul 26, 2022 • edited Loading

Andreagit97 Jul 26, 2022

Choose a reason for hiding this comment

FedeDP Jul 26, 2022

Choose a reason for hiding this comment

Andreagit97 Jul 26, 2022

Choose a reason for hiding this comment

gnosek Jul 27, 2022

Choose a reason for hiding this comment

poiana commented Jul 27, 2022

gnosek left a comment

Choose a reason for hiding this comment

gnosek Jul 27, 2022

Choose a reason for hiding this comment

Andreagit97 commented Jul 27, 2022 • edited Loading

hbrueckner commented Jul 28, 2022 • edited Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jul 28, 2022

FedeDP commented Jul 29, 2022

hbrueckner commented Jul 29, 2022

gnosek commented Jul 29, 2022

Andreagit97 commented Jul 29, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jul 29, 2022

poiana commented Jul 29, 2022

iii-i commented Jul 29, 2022

hbrueckner commented Nov 16, 2022

FedeDP commented Nov 16, 2022

Andreagit97 commented Nov 16, 2022

new(modern_bpf): add support for `open` family syscalls #503

new(modern_bpf): add support for `open` family syscalls #503

Andreagit97 commented Jul 26, 2022 •

edited

Loading

Andreagit97 commented Jul 27, 2022 •

edited

Loading

hbrueckner commented Jul 28, 2022 •

edited

Loading