Skip to content
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

info: error on loading wrong BPF object type #1581

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mtardy
Copy link
Member

@mtardy mtardy commented Oct 9, 2024

Fixes #1566.

Before this patch, ebpf.LoadPinnedMap and ebpf.LoadPinnedProg both succeed when used with the wrong object type. Meaning that you can open a prog using epbf.LoadPinnedMap or vice-versa and you'll get garbage data in the object's info.

To my knowledge, there are two ways of knowing from an opened FD which object type it is:

  • checking for specific fields in /proc/self/fdinfo/;
  • running readlink(2) on /proc/self/fd/ and check for anon_inode:bpf-prog or anon_inode:bpf-map.

This is a breaking change for users relying on LoadPinned to open any object: I've been using that behavior to scan BPF filesystem.

Note to the reviewer: I've been writing this fairly quickly, I'm sure I'm breaking all your conventions style-wise and I was very hesitant to return an error on failing to run readlink. Please feel free to comment on anything, I just wanted to start with something.

Before this patch, ebpf.LoadPinnedMap and ebpf.LoadPinnedProg both
succeed when used with the wrong object type. Meaning that you can open
a prog using epbf.LoadPinnedMap or vice-versa and you'll get garbage
data in the object's info.

To my knowledge, there are two ways of knowing from an opened FD which
object type it is:
* checking for specific fields in /proc/self/fdinfo/<fd>;
* running readlink(2) on /proc/self/fd/<fd> and check for
  anon_inode:bpf-prog or anon_inode:bpf-map.

This is a breaking change for users relying on LoadPinned<object> to
open any object: I've been using that behavior to scan BPF filesystem.

Signed-off-by: Mahe Tardy <[email protected]>
@mejedi
Copy link
Contributor

mejedi commented Oct 11, 2024

It looks like programs with capabilities can’t access their own /proc/self: https://dxuuu.xyz/filecaps.html

I wonder if we can figure a type differently.

@mtardy
Copy link
Member Author

mtardy commented Oct 15, 2024

It looks like programs with capabilities can’t access their own /proc/self: https://dxuuu.xyz/filecaps.html

I wonder if we can figure a type differently.

But isn't c/ebpf already parsing stuff from /proc/self anyway?

@mejedi
Copy link
Contributor

mejedi commented Oct 15, 2024

But isn't c/ebpf already parsing stuff from /proc/self anyway?

I wish it didn't.

It is quite reasonable to run ebpf stuff as non-root user, selectively granting needed capabilities. This will render /proc/self inaccessible.

At the moment, there are two uses of /proc/self — in info.go and internal/linux/vdso.go. The later is quite benign — it is used to determine the kernel version, but the failure is not considered fatal. The library doesn't rely on kernel version for feature detection anyway.

It appears that .Info() on ebpf.Map (but not on ebpf.Program) does not work when run by non-root user with capabilities.

The changes you propose will also prevent access to pinned objects.

I have to admit that I don't understand how to use BPF_OBJ_GET_INFO_BY_FD safely when we aren't sure about the fd type. BPF_OBJ_GET doesn't have a way to specify the expected object type. @dylandreimerink

@dylandreimerink
Copy link
Member

I have to admit that I don't understand how to use BPF_OBJ_GET_INFO_BY_FD safely when we aren't sure about the fd type. BPF_OBJ_GET doesn't have a way to specify the expected object type.

We essentially just get a byte slice back. The trouble is that without knowledge of the BPF object type there is no sure way to know how to decode it properly. We know how big we expect the output to be, but it actually differs by kernel, older kernels will have less fields so a smaller buffer.

So I wouldn't say there is anything inherently unsafe but its currently possible for users to open a pin and get bogus data. It would be nice if we can throw an error telling a user they made a mistake somewhere.

It looks like programs with capabilities can’t access their own /proc/self: https://dxuuu.xyz/filecaps.html

That is very sad. Perhaps we could still add the check, but ignore permission failures to /proc/self. Perhaps some checks are better than no checks? Difficult...

I wonder if we can figure a type differently.

So far we have not figured out another way. It would likely require a kernel change, which isn't a bad idea, but also doesn't help us right now.

We might be able to verify the info we get back from BPF_OBJ_GET_INFO_BY_FD. For example by checking if BTF IDs are plausible or data is written to pointers for name for example. (just an idea at this point)

@mejedi
Copy link
Contributor

mejedi commented Oct 16, 2024

@dylandreimerink
Thank you for sharing your thoughs.

I have to admit that I don't understand how to use BPF_OBJ_GET_INFO_BY_FD safely when we aren't sure about the fd type.

We essentially just get a byte slice back.

Some bytes in the blob we submit are interpreted as a pointer to buffer and other bytes tell the capacity. Imagine we are passing a wrong kind of blob. Can we communicate the capacity wrong and end up with a buffer overrun? Probably not as pointer/capacity fields do not align in different kinds of info structs.

I wonder if we can figure a type differently.

So far we have not figured out another way.

We could probably identify a map by doing lookup_elem with a NULL buffer. To tell if fd is a program, we could add it to a brand new PROG_ARRAY_MAP. Not sure about links. Are there any other kinds of objects that can come from bpffs?

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.

LoadPinned* silently succeeds on objects of wrong type
3 participants