On Tue, Dec 5, 2023 at 8:31 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Fri, Dec 01, 2023 at 09:47:29AM +0000, Jie Jiang wrote:
> > Parse uid and gid in bpf_parse_param() so that they can be passed in as
> > the `data` parameter when mount() bpffs. This will be useful when we
> > want to control which user/group has the control to the mounted bpffs,
> > otherwise a separate chown() call will be needed.
> >
> > Signed-off-by: Jie Jiang <jiejiang@xxxxxxxxxxxx>
> > ---
>
> Sorry, I was asked to take a quick look at this. The patchset looks fine
> overall but it will interact with Andrii's patchset which makes bpffs
> mountable inside a user namespace (with caveats).
>
> At that point you need additional validation in bpf_parse_param(). The
> simplest thing would probably to just put this into this series or into
> @Andrii's series. It's basically a copy-pasta from what I did for tmpfs
> (see below).
>
> I plan to move this validation into the VFS so that {g,u}id mount
> options are validated consistenly for any such filesystem. There is just
> some unpleasantness that I have to figure out first.
>
> @Andrii, with the {g,u}id mount option it means that userns root can
>
> fsconfig(..., FSCONFIG_SET_STRING, "uid", "1000", ...)
> fsconfig(..., FSCONFIG_SET_STRING, "gid", "1000", ...)
> fsconfig(..., FSCONFIG_CMD_CREATE, ...)
>
> If you delegate CAP_BPF in that userns to uid 1000 then an unpriv user
> in that userns can create bpf tokens. Currently this would require
> userns root to give both CAP_DAC_READ_SEARCH and CAP_BPF to such an
> unprivileged user.
This is probably fine. Basically the only difference is that BPF FS
can be instantiated inside an unpriv namespace, instead of in a
privileged parent namespace, right?
But delegate_xxx options are still guarded by the explicit
capable(CAP_SYS_ADMIN) check, so that unprivileged user won't be able
to grant themselves BPF token-enabling capabilities without a
privileged parent doing it on their behalf.
Is my understanding correct or am I missing some nuance here?
>
> Depending on whether or not that's intended you might want to add an
> additional check into bpf_token_create() to verify that the caller's
> {g,u}id resolves to 0:
>
> if (from_kuid(current_user_ns(), current_fsuid()) != 0)
> return -EINVAL;
>
> That's basically saying you're restricting this to userns root. Idk,
> that's up to you. (Note that you currently enforce current_user_ns() ==
> token->user_ns == s_user_ns which is why it doesn't matter what userns
> you pass here. You'd just error out later.)
>
> > kernel/bpf/inode.c | 33 +++++++++++++++++++++++++++++++--
> > 1 file changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> > index 1aafb2ff2e953..826fe48745ee2 100644
> > --- a/kernel/bpf/inode.c
> > +++ b/kernel/bpf/inode.c
[...]
