On Wed, Dec 6, 2023 at 2:58 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> 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.
Ok, thanks, then it seems like it's all good w.r.t. interaction with
delegate_xxx options and BPF token creation.
