On Tue, Dec 05, 2023 at 10:28:39AM -0800, Andrii Nakryiko wrote:
> 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?
Hm, I think this is slightly misphrased but I guess I get what you mean.
Basically, userns root can change what {g,u}id bpffs will use to
instantiate inodes once init_user_ns root creates the superblock. IOW,
the {g,u}id mount option isn't guarded and can thus be changed by userns
root.
>
> 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?
No, that's correct.
