-
Notifications
You must be signed in to change notification settings - Fork 4.1k
libbpf-tools: Fix ringbuf leaks that leak prior events to userspace #5506
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,4 +42,16 @@ static __always_inline long submit_buf(void *ctx, void *buf, __u64 size) | |
| return bpf_perf_event_output(ctx, &events, BPF_F_CURRENT_CPU, buf, size); | ||
| } | ||
|
|
||
| /* Zero a buffer returned by reserve_buf so callers don't have to write every | ||
| * field of the event struct, and so cross-record stack/ringbuf residue is | ||
| * not emitted to userspace. __noinline lands as a single BPF-to-BPF | ||
| * subprogram shared across call sites; volatile defeats LLVM's loop-idiom | ||
| * recognition, which would otherwise re-lower this back to __builtin_memset | ||
| * (which the BPF backend cannot inline for buffers larger than ~256 bytes). */ | ||
| static __noinline void zero_buf(void *p, __u64 sz) | ||
| { | ||
| for (__u64 i = 0; i < sz; i++) | ||
| *(volatile char *)((char *)p + i) = 0; | ||
|
Comment on lines
+49
to
+54
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ekyooo I can commit this optimization if you think it is necessary. |
||
| } | ||
|
|
||
| #endif /* __COMPAT_BPF_H */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ static int probe_exit(void *ctx, int ret) | |
| eventp = reserve_buf(sizeof(*eventp)); | ||
| if (!eventp) | ||
| goto cleanup; | ||
| zero_buf(eventp, sizeof(*eventp)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For mountsnoop, userspace already accesses only the active union arm via switch(e->op), so stale bytes in inactive arms are never read. Is "previously emitted record leak" the right framing for this case? Would it be simpler to check the return value of bpf_probe_read_user_str and skip submit_buf on failure, rather than zeroing the whole buffer? Any case where a partial event is still useful?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale bytes are never read in the current userspace program. However, the ringbuf does contain stale bytes from prior events that are accessible from userspace. Any other consumer of the same ringbuf (e.g., a custom loader) sees the full record submitted to userspace, not just the bytes in the active arm. I confirmed the leak empirically by reading the ringbuf directly (i.e., not through Regarding checking the return value of Partial events might be useful, since they are better than silently dropping the event. However, I believe that is a separate question since it would not fix the leak.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Your observation is valid, and this seems to be an expected result of mountsnoop’s design.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are right. It is not a leak in the security sense, i.e., not a KALSR-breaking leak. The information that is left over and submitted to the ringbuf is not privileged, and it was sent to userspace. It concerns a (potential) confidentiality issue (e.g., residue from a record about one mount/user can surface in a later record about a different mount/user) and general output-sanitization hygiene. Additionally, it might be problematic if we have custom userspace loaders and multiple userspace loaders accessing the same pinned ringbuf. Regarding other programs, I also observed something similar in opensnoop where the file paths, which are of variable length, contain stale bytes from previous filepaths that were longer. |
||
|
|
||
| task = (struct task_struct *)bpf_get_current_task(); | ||
| eventp->delta = bpf_ktime_get_ns() - argp->ts; | ||
|
|
||
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.
Two concerns with zero_buf():
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.
Sorry, I missed this.
zero_bufadds? Also, another way to speed things up would be to zero out only the hole in the struct. If we do this, we have will have optimized ringbuf zero functions for each eBPF program that leaks data.