Skip to content

tetragon: Get proper exex cwd in case of no arguments #683

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

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Feb 5, 2023

If there are no arguments (api.EventDataArgs is not set) we trash the CWD string which follows arguments.

This can be reproduced by forcing arguments not to be stored like with following change:

  --- a/bpf/process/bpf_execve_event.c
  +++ b/bpf/process/bpf_execve_event.c
  @@ -51,7 +51,7 @@ event_args_builder(void *ctx, struct msg_execve_event *event)
          /* We use flags in asm to indicate overflow */
          compiler_barrier();
          probe_read(&mm, sizeof(mm), _(&task->mm));
  -       if (mm) {
  +       if (0 && mm) {

I'm not sure at the moment how to make test for this.

Signed-off-by: Jiri Olsa [email protected]

@olsajiri olsajiri marked this pull request as ready for review February 5, 2023 21:57
@olsajiri olsajiri requested a review from a team as a code owner February 5, 2023 21:57
@olsajiri olsajiri requested a review from tixxdz February 5, 2023 21:57
@jrfastab
Copy link
Contributor

jrfastab commented Feb 5, 2023

happen to know when we introduced this? Did CWD really never work without args? Lots of execs don't push args I suspect.

@olsajiri
Copy link
Contributor Author

olsajiri commented Feb 6, 2023

I don't think it would trigger that, we read args with:

1) read task->mm
2) read mm->arg_start and mm->arg_end
3) read/skip binary from arg_start (first arg)
4) read rest of the args if there's stil space event

so kernel tasks without mm could reproduce that.. the rest would need some read error later in the code path
also hubble does not have this problem.. for some reason this hunk that removed the else path was not backported (thanks @tixxdz for spotting that)

the commit time of the change is Aug 19 2022, but I think it got merged later than that

Using processapi module handle in exec_test.go so we can
add and use 'api' handle in following changes.

Signed-off-by: Jiri Olsa <[email protected]>
Adding DataAdd and DataPurge functions for observer's data cache,
so it can be used later in test.

Signed-off-by: Jiri Olsa <[email protected]>
Adding tests for directly for execParse function for special
cases (normal ones as well) of execve event's data like filename,
arguments and cwd.

Suggested-by: Kornilios Kourtis <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
If there are no arguments (api.EventDataArgs is not set)
we trash the CWD string which follows arguments.

Signed-off-by: Jiri Olsa <[email protected]>
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Thanks!

Copy link
Member

@tixxdz tixxdz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thanks!

@tixxdz tixxdz merged commit 1a68bf2 into cilium:main Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants