fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

incertum · 2022-09-26T03:57:26Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

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:

Reproduce bug and verify fix:

Currently in driver-modern-bpf we do not get some execve data, for example:

sudo ./libsinsp/examples/sinsp-example --modern_bpf -f "evt.dir=< and (evt.type=execve or evt.type=execveat)" -o "* %proc.cmdline %proc.exepath %proc.name %proc.env"

Current master:

{"proc.cmdline":"git","proc.env":"","proc.exepath":"/usr/bin/git","proc.name":"git"}

After reverting KERNEL reads to bpf_probe_read or bpf_probe_read_str everything seems to be fixed again (this PR):

{"proc.cmdline":"git status","proc.env":"LONG REDACTED STRING","proc.exepath":"/usr/bin/git","proc.name":"git"}

Checked in some other projects and distinguishing between KERNEL and USER reads is best practice. However, I also noticed that while _user bpf helpers are used, the _kernel helpers are not used and instead the global ones are used. Therefore, I suspect it is the actual bpf helpers causing issues here. No idea what it could be.

Edited: Actual issue was some wrong USER vs MEMORY reads attribution in execve filler.

CC folks who touched modern_bpf so far: @Andreagit97 @hbrueckner @Molter73 @FedeDP @jasondellaluce @leogr

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

…t success retval * bpf_probe_read and bpf_probe_read_user used in push__bytebuf do not return bytes but success retval, refactor to reflect this better. * (char *)bytebuf_pointer -> (void *)bytebuf_pointer) Signed-off-by: Melissa Kilby <[email protected]>

Signed-off-by: Melissa Kilby <[email protected]>

Andreagit97 · 2022-09-26T06:45:46Z

Hey @incertum thank you for this I will take a look ASAP :)
Probably we cannot revert the changes like this because we would broken the s390x support, but yes we need to understand what's is going on here

Andreagit97 · 2022-09-26T07:31:03Z

Just checked @incertum the fix should be simple than this, we need to change all KERNEL in USER like here https://github.com/falcosecurity/libs/blob/master/driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c#L82
We need to do this for params 2, 3 and 16 like we do in the clone.bpf.c

hbrueckner · 2022-09-26T13:36:47Z

Thanks @Andreagit97

Just checked @incertum the fix should be simple than this, we need to change all KERNEL in USER like here https://github.com/falcosecurity/libs/blob/master/driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c#L82 We need to do this for params 2, 3 and 16 like we do in the clone.bpf.c

The arg_start and arg_end, respectively for environment as well... point to user space memory. So you are right, that we need to use USER in those places.

hbrueckner · 2022-09-26T13:41:06Z

Hi @incertum

Hey @incertum thank you for this I will take a look ASAP :) Probably we cannot revert the changes like this because we would broken the s390x support, but yes we need to understand what's is going on here

Distinguishing between kernel and user space pointers is essential for s390x. On s390x, kernel and user space have their own address space and, hence, address can overlap. I opened #497 some time ago for the BPF driver and did a couple of reviews on modern BPF in this regard.

Collectively verified that this fixes issues in execve params 2, 3 and 16. Co-authored-by: Andrea Terzolo <[email protected]> Co-authored-by: Hendrik Brueckner <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

incertum · 2022-09-26T14:33:58Z

@Andreagit97 and @hbrueckner I like simpler fixes that actually make sense 😄 , squashed and adjusted last commit accordingly. Thank you!

hbrueckner · 2022-09-26T14:51:44Z

driver/modern_bpf/helpers/base/push_data.h

@@ -253,11 +256,20 @@ static __always_inline u16 push__charbuf(u8 *data, u64 *payload_pos, unsigned lo
 */
 static __always_inline u16 push__bytebuf(u8 *data, u64 *payload_pos, unsigned long bytebuf_pointer, u16 len_to_read, enum read_memory mem)
 {
+	u16 total_len = 0;
+	if (len_to_read > ARGS_ENV_SIZE_MAX)


only a nit: Maybe it is worth to have some min/max macros here? (If so, maybe a follow-up PR).

@hbrueckner could skip reading at all if len_to_read = 0

@hbrueckner pushed a simple suggestion, let me know, can adjust it.

@hbrueckner could skip reading at all if len_to_read = 0

The code is already doing it, if len_to_read=0 we push 0

Added benefit in the modern_bpf API setup is that we exit faster here. Thought @hbrueckner raised a valid point, always be consistent and do min/max checks if you do max checks.

Uhm let's say here there is the usual trade-off:

save one bpf_probe_read in case the len_to_read is 0. (not frequent scenario)

add complexity and pay for the checks when len_to_read is !=0. (frequent scenario).

That's exactly the trade-off we have in the old probe, very complex code to save corner cases like this, to be honest, I would avoid this check at least for now, if we notice a real benefit from this we can always introduce it in a second step 🤔

hbrueckner · 2022-09-26T14:52:29Z

Hi @incertum

@Andreagit97 and @hbrueckner I like simpler fixes that actually make sense smile , squashed and adjusted last commit accordingly. Thank you!

I had also the same patch around :-)
/lgtm

poiana · 2022-09-26T14:52:35Z

LGTM label has been added.

Git tree hash: 880f8010ecd09f0f6d508b3558eae30de14610f8

Andreagit97 · 2022-09-26T14:57:29Z

driver/modern_bpf/helpers/base/push_data.h

@@ -89,6 +89,9 @@ enum read_memory
 /* Paramater maximum size */
 #define MAX_PARAM_SIZE MAX_EVENT_SIZE - 1

+/* ARGS or ENV maximum size */
+#define ARGS_ENV_SIZE_MAX 4096


not completely understood why we need this 🤔

SAFE_ACCESS should already protect us from the worst case

It has been part of the old probe and it may be less for protection and rather more for you don't wanna ever log super super long unbounded strings in your logs. @leogr @FedeDP do you have more insights re what the motivation was in the old probe? Only safe reads or also some guarantees on limits? Lastly for push__charbuf limits are enforced so it would also be about consistency.

It has been part of the old probe and it may be less for protection and rather more for you don't wanna ever log super super long unbounded strings in your logs.

That's a good point for this reason we use approaches like the snaplen to let users understand the max dimension they want to send, you can see an example here

libs/driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/sendto.bpf.c

Lines 89 to 98 in 0c280ca

unsigned long bytes_to_read = maps__get_snaplen();

if(bytes_to_read > ret)

{

bytes_to_read = ret;

}

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

unsigned long sent_data_pointer = extract__syscall_argument(regs, 1);

auxmap__store_bytebuf_param(auxmap, sent_data_pointer, bytes_to_read, USER);

The idea of this snaplen concept is to limit at an higher level so you can chose when to limit rather then implementing it directly at low level

For what concerns the old probe we use this same concept of snaplen all the others checks are to please the verifier AFAIK 🤔

Those are the two relevant max bounds from old probe

#define ARGS_ENV_SIZE_MAX 4096 #define MAX_PATH_LENGTH 4096

These values are battle proven, so wouldn't go higher before testing modern_bpf in the field.

Probably have a few options here and we can go step-wise, no need to change all now.

Could start with uniformly enforcing bounds to existing MAX_PARAM_SIZE, if I can say so, the value appears extremely large, I think 2^12 is reasonable and enough and as mentioned battle proven. If we go this route can change this quickly and reuse the existing macro.

If we need more granular upper bounds can introduce a new enum, similarly to the read_memory. For example, for the cmd args I can see that maybe 2^13 could be a good choice for an enforced upper bound. Would however perform all bound checks and selecting the corresponding upper bound here and not in the fillers. In addition, fillers can always also enforce a lower value than the max enforced bound.

These bound checks are not unique to Falco or libs, seeing them everywhere and I would be very hesitant deploying anything to production without them.

Sorry typo meant the MAX_PARAM_SIZE you also use in SAFE_ACCESS

uhm I got your point maybe MAX_PARAM_SIZE is big enough, it's the same value we use in the old probe but it could be dangerous...

/* Event maximum size */ #define MAX_EVENT_SIZE 64 * 1024 /* Paramater maximum size */ #define MAX_PARAM_SIZE MAX_EVENT_SIZE - 1

BTW putting a hard-coded value here doesn't seem the right solution, or better let's say it could be the right solution if we were in production and we have to rush, but this is not the case 🤣 let's take our time to think of something more dynamic or configurable

I've added some dedicated checks for execve here https://github.com/falcosecurity/libs/pull/648/files#diff-23e1e00c90562960e7e4110fcc5c62cd89d2ea8a8df3198d7c04512084c7c378R52-R54 :)

…n_to_read Co-authored-by: Hendrik Brueckner <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

FedeDP

Hi! I'd split the feature/fix ARGS_ENV_SIZE_MAX from the real fix, and open a PR just for that, if you agree!
The fix is just the driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c diff, right?

hbrueckner · 2022-10-04T15:51:00Z

Hi @incertum , @FedeDP , @Andreagit97 ,

Hi! I'd split the feature/fix ARGS_ENV_SIZE_MAX from the real fix, and open a PR just for that, if you agree! The fix is just the driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c diff, right?

trying to catch up on this and #648, my thoughts tend to have the ARGS_ENV_SIZE_MAX in #648 (with a co-authored-by @incertum ). The refactoring towards the collecting execve args/env and passing the maximum there is consistent with MAX_PARAM_SIZE and MAX_UNIX_SOCKET_PATH. And, I guess, this is for now limited to execve and execveat (the other process-creating syscalls always pass zero for now).

On 7bf2082](7bf2082), checking len_to_read < 1 is fine for this PR.

Wdyt?

Andreagit97 · 2022-10-04T16:04:59Z

trying to catch up on this and #648, my thoughts tend to have the ARGS_ENV_SIZE_MAX in #648 (with a co-authored-by @incertum ). The refactoring towards the collecting execve args/env and passing the maximum there is consistent with MAX_PARAM_SIZE and MAX_UNIX_SOCKET_PATH. And, I guess, this is for now limited to execve and execveat (the other process-creating syscalls always pass zero for now).

Sounds good :)

On (7bf2082), checking len_to_read < 1 is fine for this PR.

Yeah let's say i have only one concern about this, that's what we discovered in #648... a simple difference in how we do stuff can lead to a perf penalty not negligible, i don't think this is the case because we have just an if, but we have to say that the only gain from this if is that we don't call bpf_probe_read_kernel helper, so we save perf only in corner cases 🤔 BTW, if you think this can be valuable we can do that :)

* move lower bound check for push__bytebuf to auxmap__store_bytebuf_param * address upper bound checks in follow up PRs Co-authored-by: Hendrik Brueckner <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

incertum · 2022-10-04T23:52:45Z

Hoping new commit reflects a balance @hbrueckner @Andreagit97 @FedeDP. Small note in favor of keeping the lower bound check: no cmd args is not necessarily an edge case, especially when considering clone/fork family of syscalls. Basing this statement on some experimental observations. However, can always remove the lower bound check in the future if performance tests surface that alternatives are better.

hbrueckner · 2022-10-05T13:28:40Z

Hi @incertum ,

Hoping new commit reflects a balance @hbrueckner @Andreagit97 @FedeDP.

Commit looks fine to me.

Small note in favor of keeping the lower bound check: no cmd args is not necessarily an edge case, especially when considering clone/fork family of syscalls. Basing this statement on some experimental observations. However, can always remove the lower bound check in the future if performance tests surface that alternatives are better.

Makes sense. Perhaps there is also a way higher in the stack to get some statistics on the such events.

/lgtm

poiana · 2022-10-05T14:10:00Z

LGTM label has been added.

Git tree hash: 43a4554e6357ca72e02cc311318429d8a5b28b12

Andreagit97 · 2022-10-05T17:51:12Z

/hold

Andreagit97

Thank you for these changes :)
/approve

FedeDP

/approve

poiana · 2022-10-13T07:43:40Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

/unhold

incertum added 2 commits September 25, 2022 20:16

cleanup(driver-modern-bpf): enforce upper bounds for push__bytebuf

df17037

Signed-off-by: Melissa Kilby <[email protected]>

poiana added kind/bug Something isn't working release-note-none dco-signoff: yes kind/cleanup area/driver-modern-bpf labels Sep 26, 2022

poiana requested review from FedeDP and hbrueckner September 26, 2022 03:57

poiana added the size/M label Sep 26, 2022

incertum force-pushed the fix-modern-bpf-push-data branch from 57d7ff1 to a94d950 Compare September 26, 2022 14:31

incertum changed the title ~~fix(driver-modern-bpf): Revert bpf_probe_read_kernel* bpf helpers + cleanup + enforce upper bounds in push__bytebuf~~ fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf Sep 26, 2022

hbrueckner reviewed Sep 26, 2022

View reviewed changes

poiana assigned hbrueckner Sep 26, 2022

poiana added the lgtm label Sep 26, 2022

Andreagit97 reviewed Sep 26, 2022

View reviewed changes

cleanup(driver-modern-bpf): skip push__bytebuf reads when no valid le…

7bf2082

…n_to_read Co-authored-by: Hendrik Brueckner <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

poiana removed the lgtm label Sep 26, 2022

poiana requested a review from hbrueckner September 26, 2022 18:00

Andreagit97 added this to the 0.10.0 milestone Sep 27, 2022

FedeDP reviewed Oct 4, 2022

View reviewed changes

hbrueckner mentioned this pull request Oct 4, 2022

update(modern_bpf): reduce the execve instrumentation time with new APIs #648

Merged

poiana added the lgtm label Oct 5, 2022

poiana added the do-not-merge/hold label Oct 5, 2022

Andreagit97 approved these changes Oct 5, 2022

View reviewed changes

poiana assigned Andreagit97 Oct 5, 2022

poiana added the approved label Oct 5, 2022

FedeDP approved these changes Oct 13, 2022

View reviewed changes

poiana assigned FedeDP Oct 13, 2022

poiana removed the do-not-merge/hold label Oct 13, 2022

poiana merged commit 099392c into falcosecurity:master Oct 13, 2022

incertum deleted the fix-modern-bpf-push-data branch December 8, 2023 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

incertum commented Sep 26, 2022 •

edited

Loading

Andreagit97 commented Sep 26, 2022 •

edited

Loading

Andreagit97 commented Sep 26, 2022

hbrueckner commented Sep 26, 2022

hbrueckner commented Sep 26, 2022

incertum commented Sep 26, 2022

hbrueckner Sep 26, 2022

incertum Sep 26, 2022

incertum Sep 26, 2022

Andreagit97 Sep 26, 2022 •

edited

Loading

incertum Sep 27, 2022

Andreagit97 Oct 1, 2022

hbrueckner commented Sep 26, 2022

poiana commented Sep 26, 2022

Andreagit97 Sep 26, 2022

Andreagit97 Sep 26, 2022

incertum Sep 26, 2022

Andreagit97 Sep 26, 2022

incertum Sep 27, 2022 •

edited

Loading

incertum Sep 27, 2022

Andreagit97 Oct 1, 2022

Andreagit97 Oct 4, 2022

FedeDP left a comment •

edited

Loading

hbrueckner commented Oct 4, 2022

Andreagit97 commented Oct 4, 2022

incertum commented Oct 4, 2022

hbrueckner commented Oct 5, 2022

poiana commented Oct 5, 2022

Andreagit97 commented Oct 5, 2022

Andreagit97 left a comment

FedeDP left a comment

poiana commented Oct 13, 2022

Andreagit97 commented Oct 13, 2022

	unsigned long bytes_to_read = maps__get_snaplen();

	if(bytes_to_read > ret)
	{
	bytes_to_read = ret;
	}

	/* Parameter 2: data (type: PT_BYTEBUF) */
	unsigned long sent_data_pointer = extract__syscall_argument(regs, 1);
	auxmap__store_bytebuf_param(auxmap, sent_data_pointer, bytes_to_read, USER);

fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

Conversation

incertum commented Sep 26, 2022 • edited Loading

Andreagit97 commented Sep 26, 2022 • edited Loading

Andreagit97 commented Sep 26, 2022

hbrueckner commented Sep 26, 2022

hbrueckner commented Sep 26, 2022

incertum commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbrueckner commented Sep 26, 2022

poiana commented Sep 26, 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

incertum Sep 27, 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

FedeDP left a comment • edited Loading

Choose a reason for hiding this comment

hbrueckner commented Oct 4, 2022

Andreagit97 commented Oct 4, 2022

incertum commented Oct 4, 2022

hbrueckner commented Oct 5, 2022

poiana commented Oct 5, 2022

Andreagit97 commented Oct 5, 2022

Andreagit97 left a comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 13, 2022

Andreagit97 commented Oct 13, 2022

incertum commented Sep 26, 2022 •

edited

Loading

Andreagit97 commented Sep 26, 2022 •

edited

Loading

Andreagit97 Sep 26, 2022 •

edited

Loading

incertum Sep 27, 2022 •

edited

Loading

FedeDP left a comment •

edited

Loading