Hi Andy, > On Jun 27, 2019, at 4:40 PM, Andy Lutomirski <luto@xxxxxxxxxx> wrote: > > On 6/27/19 1:19 PM, Song Liu wrote: >> This patch introduce unprivileged BPF access. The access control is >> achieved via device /dev/bpf. Users with write access to /dev/bpf are able >> to call sys_bpf(). >> Two ioctl command are added to /dev/bpf: >> The two commands enable/disable permission to call sys_bpf() for current >> task. This permission is noted by bpf_permitted in task_struct. This >> permission is inherited during clone(CLONE_THREAD). >> Helper function bpf_capable() is added to check whether the task has got >> permission via /dev/bpf. > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 0e079b2298f8..79dc4d641cf3 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -9134,7 +9134,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, >> env->insn_aux_data[i].orig_idx = i; >> env->prog = *prog; >> env->ops = bpf_verifier_ops[env->prog->type]; >> - is_priv = capable(CAP_SYS_ADMIN); >> + is_priv = bpf_capable(CAP_SYS_ADMIN); > > Huh? This isn't a hardening measure -- the "is_priv" verifier mode allows straight-up leaks of private kernel state to user mode. > > (For that matter, the pending lockdown stuff should possibly consider this a "confidentiality" issue.) > > > I have a bigger issue with this patch, though: it's a really awkward way to pretend to have capabilities. For bpf, it seems like you could make this be a *real* capability without too much pain since there's only one syscall there. Just find a way to pass an fd to /dev/bpf into the syscall. If this means you need a new bpf_with_cap() syscall that takes an extra argument, so be it. The old bpf() syscall can just translate to bpf_with_cap(..., -1). > > For a while, I've considered a scheme I call "implicit rights". There would be a directory in /dev called /dev/implicit_rights. This would either be part of devtmpfs or a whole new filesystem -- it would *not* be any other filesystem. The contents would be files that can't be read or written and exist only in memory. You create them with a privileged syscall. Certain actions that are sensitive but not at the level of CAP_SYS_ADMIN (use of large-attack-surface bpf stuff, creation of user namespaces, profiling the kernel, etc) could require an "implicit right". When you do them, if you don't have CAP_SYS_ADMIN, the kernel would do a path walk for, say, /dev/implicit_rights/bpf and, if the object exists, can be opened, and actually refers to the "bpf" rights object, then the action is allowed. Otherwise it's denied. > > This is extensible, and it doesn't require the rather ugly per-task state of whether it's enabled. > > For things like creation of user namespaces, there's an existing API, and the default is that it works without privilege. Switching it to an implicit right has the benefit of not requiring code changes to programs that already work as non-root. > > But, for BPF in particular, this type of compatibility issue doesn't exist now. You already can't use most eBPF functionality without privilege. New bpf-using programs meant to run without privilege are *new*, so they can use a new improved API. So, rather than adding this obnoxious ioctl, just make the API explicit, please. > > Also, please cc: linux-abi next time. Thanks for your inputs. I think we need to clarify the use case here. In this case, we are NOT thinking about creating new tools for unprivileged users. Instead, we would like to use existing tools without root. Currently, users of these tools have to run them with root or sudo. But they would prefer not to. On the kernel side, we are not planning provides a subset of safe features for unprivileged users. The permission here is all-or-nothing. Introducing bpf_with_cap() syscall means we need teach these tools to manage the fd, and use the new API when necessary. This is clearly not easy. On the other hand, current solution is easy to adopt for most tools (see 4/4 of this set). Also, for this use case, I don't see bpf_with_cap() more secure than this patch. Thanks, Song
