-
Notifications
You must be signed in to change notification settings - Fork 699
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
base: main
Are you sure you want to change the base?
Conversation
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]>
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 |
I wish it didn't. It is quite reasonable to run ebpf stuff as non-root user, selectively granting needed capabilities. This will render At the moment, there are two uses of It appears that The changes you propose will also prevent access to pinned objects. I have to admit that I don't understand how to use |
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.
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...
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 |
@dylandreimerink
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.
We could probably identify a map by doing |
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:
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.