| Message ID | 20210618105526.265003-1-zenczykowski@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] Revert "bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET" | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: yhs@fb.com kpsingh@kernel.org kafai@fb.com john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > This breaks Android userspace which expects to be able to > fetch programs with just read permissions. Sorry about this! I'll defer to the maintainers what to do here. Reverting leaves us with a gaping hole for access control of pinned programs.
On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski > <zenczykowski@gmail.com> wrote: > > > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > > > This breaks Android userspace which expects to be able to > > fetch programs with just read permissions. > > Sorry about this! I'll defer to the maintainers what to do here. > Reverting leaves us with a gaping hole for access control of pinned > programs. Not sure what hole you're referring to. Could you provide more details/explanation? It seems perfectly reasonable to be able to get a program with just read privs. After all, you're not modifying it, just using it. AFAIK there is no way to modify a program after it was loaded, has this changed? if so, the checks should be on the modifications not the fd fetch. I guess one could argue fetching with write only privs doesn't make sense? Anyway... userspace is broken... so revert is the answer. In Android the process loading/pinning bpf maps/programs is a different process (the 'bpfloader') to the users (which are far less privileged)
On Fri, 18 Jun 2021 at 19:30, Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > > On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski > > <zenczykowski@gmail.com> wrote: > > > > > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > > > > > This breaks Android userspace which expects to be able to > > > fetch programs with just read permissions. > > > > Sorry about this! I'll defer to the maintainers what to do here. > > Reverting leaves us with a gaping hole for access control of pinned > > programs. > > Not sure what hole you're referring to. Could you provide more details/explanation? > > It seems perfectly reasonable to be able to get a program with just read privs. > After all, you're not modifying it, just using it. Agreed, if that was what the kernel is doing. What you get with BPF_F_RDONLY is a fully read-write fd, since the rest of the BPF subsystem doesn't check program fd flags. Hence my fix to only allow O_RDWR, which matches what the kernel actually does. Otherwise any user with read-only access can get a R/W fd. > AFAIK there is no way to modify a program after it was loaded, has this changed? You can't modify the program, but you can detach it, for example. Any program related bpf command that takes a program fd basically. > if so, the checks should be on the modifications not the fd fetch. True, unfortunately that code doesn't exist. It's also not straightforward to write and probably impossible to backport. > I guess one could argue fetching with write only privs doesn't make sense? > > Anyway... userspace is broken... so revert is the answer. > > In Android the process loading/pinning bpf maps/programs is a different > process (the 'bpfloader') to the users (which are far less privileged) If the revert happens you need to make sure that all of your pinned state is only readable by the bpfloader user. And everybody else, realistically.
The patch this reverts breaks (Android) userspace, and I've even pointed out the specific code in question that it breaks. What happened to the policy of not breaking userspace? Why are 'Changes Requested' (see https://patchwork.kernel.org/project/netdevbpf/patch/20210618105526.265003-1-zenczykowski@gmail.com/ ) by whom? What changes? What do you expect me to do? Why should I even care? Why should it even be me? I'm not the one that broke things. On Mon, Jun 21, 2021 at 2:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Fri, 18 Jun 2021 at 19:30, Maciej Żenczykowski > <zenczykowski@gmail.com> wrote: > > > > On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > > > > On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski > > > <zenczykowski@gmail.com> wrote: > > > > > > > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > > > > > > > This breaks Android userspace which expects to be able to > > > > fetch programs with just read permissions. > > > > > > Sorry about this! I'll defer to the maintainers what to do here. > > > Reverting leaves us with a gaping hole for access control of pinned > > > programs. > > > > Not sure what hole you're referring to. Could you provide more details/explanation? > > > > It seems perfectly reasonable to be able to get a program with just read privs. > > After all, you're not modifying it, just using it. > > Agreed, if that was what the kernel is doing. What you get with > BPF_F_RDONLY is a fully read-write fd, since the rest of the BPF > subsystem doesn't check program fd flags. Hence my fix to only allow > O_RDWR, which matches what the kernel actually does. Otherwise any > user with read-only access can get a R/W fd. > > > AFAIK there is no way to modify a program after it was loaded, has this changed? > > You can't modify the program, but you can detach it, for example. Any > program related bpf command that takes a program fd basically. I fail to see how this is a problem, since it's not modifying the program, why should it need a rdwr file descriptor to do so? AFAIK in many cases, you don't even need the bpf file descriptor at all (for example you can detach a tc bpf program via removing the tc filter or tc qdisc or the network interface). Do you perhaps mean to say you can unpin it? But, if so, then that's a problem in the unpin code... Or it's even entirely unrelated, since deleting files does not need read or write access to the file, just the folder the file is in. [maze@zeus ~]$ touch foo; chmod a-rwx foo; sudo chown root:root foo; ls -al foo ----------. 1 root root 0 Jun 21 14:26 foo [maze@zeus ~]$ rm -f foo; ls -al foo ls: cannot access 'foo': No such file or directory > > if so, the checks should be on the modifications not the fd fetch. > > True, unfortunately that code doesn't exist. It's also not > straightforward to write and probably impossible to backport. Now you're suggesting you expect this broken patch (that I'm trying to revert) to make it into older LTS releases and break things out in the field??? > > I guess one could argue fetching with write only privs doesn't make sense? > > > > Anyway... userspace is broken... so revert is the answer. > > > > In Android the process loading/pinning bpf maps/programs is a different > > process (the 'bpfloader') to the users (which are far less privileged) > > If the revert happens you need to make sure that all of your pinned > state is only readable by the bpfloader user. And everybody else, > realistically. On Android selinux prevents anyone from doing untoward things, since they can't get the fds in the first place. I *want* less privileged users (that can't load bpf programs, but have the selinux privs to get pinned program fds) to be able to attach/detach them (via xdp, tc or to cgroups). That's how stuff works right *now*. Could I perhaps redesign the system to work around this? I don't know. Perhaps. Perhaps not. I haven't given it that much thought - I'm still trying to fix (workaround) an hrtimer ncm performance regression that was introduced in 5.10.~24 LTS (there at least we can argue the old code was buggy, and the hrtimer implementation on the hardware in question outright terrible and super slow). I'm guessing changes would be needed to the progRetrieve() function, and the tests, and the bpfloader, and the permissions embedded in the programs themselves, and possibly the iptables binary (and the netutils wrapper) since it uses xt_bpf, and possibly to binaries privileges and/or selinux capabilities/policies. Or maybe not. I simply don't know. Well tested patches to make things work are welcome at the aosp project. A word of warning: I haven't checked / thought things through, but it may take 5+ years to roll them out. At this point it's far too late to make such changes in Android 12/S. Here's another example of fetching programs with BPF_F_RDONLY in iptables (yes, it was added by me, due to our use in Android): https://android.googlesource.com/platform/external/iptables.git/+/refs/heads/master/extensions/libxt_bpf.c#64 The fact that this snuck into the 5.12 final release is not relevant (ie. this is a regression in 5.12 vs 5.11, and it is still broken in 5.13-rcX). Please revert immediately. I've got better things to do. I shouldn't have to be thinking about this or arguing about this. It already took me significantly more than a day simply to track this down (arguably due to miscommunications with Greg, who'd earlier actually found this in 5.12, but misunderstood the problem, but still...). Thanks, Maciej
On Mon, 21 Jun 2021 at 22:37, Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > Please revert immediately. I've got better things to do. I shouldn't > have to be thinking about this or arguing about this. > It already took me significantly more than a day simply to track this > down (arguably due to miscommunications with Greg, who'd earlier > actually found this in 5.12, but misunderstood the problem, but > still...). You're barking up the wrong tree. I don't object to reverting the patch, you asked me for context and I gave it to you. Best Lorenz
On 6/18/21 12:55 PM, Maciej Żenczykowski wrote: > From: Maciej Żenczykowski <maze@google.com> > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > This breaks Android userspace which expects to be able to > fetch programs with just read permissions. > > See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124 > > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Andrii Nakryiko <andrii@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Greg Kroah-Hartman <gregkh@google.com> > Cc: Lorenz Bauer <lmb@cloudflare.com> > Fixes: d37300ed1821 ("bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET") > Signed-off-by: Maciej Żenczykowski <maze@google.com> > --- Acked-by: Greg Kroah-Hartman <gregkh@google.com>
On 6/22/21 10:59 AM, Lorenz Bauer wrote: > On Mon, 21 Jun 2021 at 22:37, Maciej Żenczykowski > <zenczykowski@gmail.com> wrote: >> >> Please revert immediately. I've got better things to do. I shouldn't >> have to be thinking about this or arguing about this. >> It already took me significantly more than a day simply to track this >> down (arguably due to miscommunications with Greg, who'd earlier >> actually found this in 5.12, but misunderstood the problem, but >> still...). > > You're barking up the wrong tree. I don't object to reverting the > patch, you asked me for context and I gave it to you. +1, this kind of barking was unnecessary and inappropriate. I revamped the commit message a bit to have some more context for future reference when we need to get back on this. Anyway, applied.
On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > From: Maciej Żenczykowski <maze@google.com> > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > This breaks Android userspace which expects to be able to > fetch programs with just read permissions. > > See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124 As a follow up, what does Android expect to be able to do with this read only FD? Lorenz
Reposting since apparently my reply only went to Lorenz. On Wed, Jun 23, 2021 at 1:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski > <zenczykowski@gmail.com> wrote: > > > > From: Maciej Żenczykowski <maze@google.com> > > > > This reverts commit d37300ed182131f1757895a62e556332857417e5. > > > > This breaks Android userspace which expects to be able to > > fetch programs with just read permissions. > > > > See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124 > > As a follow up, what does Android expect to be able to do with this > read only FD? I'm not actually sure of all the use cases, but at a bare minimum: We use it for iptables xt_bpf, and to attach to cgroup net hooks and tc bpf hooks. There's also some still incomplete support for xdp. There's also non-networking stuff like gpu memory tracking and tracepoints that I know very little about - probably something perf related. So I think the answer is that mostly we expect to be able to attach it to places (iptables/cgroup/tc/xdp/....others...??)
> > You're barking up the wrong tree. I don't object to reverting the > > patch, you asked me for context and I gave it to you. > > +1, this kind of barking was unnecessary and inappropriate. > > I revamped the commit message a bit to have some more context for future > reference when we need to get back on this. > > Anyway, applied. Thank you for applying the change. Sorry for the tone, but work has been particularly hectic these past few quarters, with a constant onslaught of neverending deadlines... The current kernel patch management system provides virtually no feedback about (rejected) patches. There is no email when a patch is rejected (and often even when applied) and there is no notification of who did it or why. It's very much like dealing with a faceless robot. I'm stuck periodically refreshing a browser tab with https://patchwork.kernel.org/project/netdevbpf/list/ and waiting on patch state to change from new to accepted or 'changes requested'. In this particular case, there was no email feedback from any maintainer, except for the patch going into 'changes requested' state on patchworks, and the email thread (which only had me and Lorenz participating) also didn't appear to have any reason in it either.... As such, it was extremely unclear what was being asked of me. It felt like I was simply being ignored. - Maciej
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c index b4ebd60a6c16..80da1db47c68 100644 --- a/kernel/bpf/inode.c +++ b/kernel/bpf/inode.c @@ -543,7 +543,7 @@ int bpf_obj_get_user(const char __user *pathname, int flags) return PTR_ERR(raw); if (type == BPF_TYPE_PROG) - ret = (f_flags != O_RDWR) ? -EINVAL : bpf_prog_new_fd(raw); + ret = bpf_prog_new_fd(raw); else if (type == BPF_TYPE_MAP) ret = bpf_map_new_fd(raw, f_flags); else if (type == BPF_TYPE_LINK)