[PATCH v4 2/12] bpf: introduce BPF token object

Thu Oct 12 00:32:45 UTC 2023

On Tue, Oct 10, 2023 at 2:20 PM Paul Moore <paul at paul-moore.com> wrote:
>
> On Fri, Sep 22, 2023 at 6:35 PM Andrii Nakryiko
> <andrii.nakryiko at gmail.com> wrote:
> > On Thu, Sep 21, 2023 at 3:18 PM Paul Moore <paul at paul-moore.com> wrote:
> > > On Fri, Sep 15, 2023 at 4:59 PM Andrii Nakryiko
> > > <andrii.nakryiko at gmail.com> wrote:
> > > > On Thu, Sep 14, 2023 at 5:55 PM Paul Moore <paul at paul-moore.com> wrote:
> > > > > On Thu, Sep 14, 2023 at 1:31 PM Andrii Nakryiko
> > > > > <andrii.nakryiko at gmail.com> wrote:
> > > > > > On Wed, Sep 13, 2023 at 2:46 PM Paul Moore <paul at paul-moore.com> wrote:
> > > > > > >
> > > > > > > On Sep 12, 2023 Andrii Nakryiko <andrii.nakryiko at gmail.com> wrote:
>
> ...
>
> > > > > > > I mentioned this a while back, likely in the other threads where this
> > > > > > > token-based approach was only being discussed in general terms, but I
> > > > > > > think we want to have a LSM hook at the point of initial token
> > > > > > > delegation for this and a hook when the token is used.  My initial
> > > > > > > thinking is that we should be able to address the former with a hook
> > > > > > > in bpf_fill_super() and the latter either in bpf_token_get_from_fd()
> > > > > > > or bpf_token_allow_XXX(); bpf_token_get_from_fd() would be simpler,
> > > > > > > but it doesn't allow for much in the way of granularity.  Inserting the
> > > > > > > LSM hooks in bpf_token_allow_XXX() would also allow the BPF code to fall
> > > > > > > gracefully fallback to the system-wide checks if the LSM denied the
> > > > > > > requested access whereas an access denial in bpf_token_get_from_fd()
> > > > > > > denial would cause the operation to error out.
> > > > > >
> > > > > > I think the bpf_fill_super() LSM hook makes sense, but I thought
> > > > > > someone mentioned that we already have some generic LSM hook for
> > > > > > validating mounts? If we don't, I can certainly add one for BPF FS
> > > > > > specifically.
> > > > >
> > > > > We do have security_sb_mount(), but that is a generic mount operation
> > > > > access control and not well suited for controlling the mount-based
> > > > > capability delegation that you are proposing here.  However, if you or
> > > > > someone else has a clever way to make security_sb_mount() work for
> > > > > this purpose I would be very happy to review that code.
> > > >
> > > > To be honest, I'm a bit out of my depth here, as I don't know the
> > > > mounting parts well. Perhaps someone from VFS side can advise. But
> > > > regardless, I have no problem adding a new LSM hook as well, ideally
> > > > not very BPF-specific. If you have a specific form of it in mind, I'd
> > > > be curious to see it and implement it.
> > >
> > > I agree that there can be benefits to generalized LSM hooks, but in
> > > this hook I think it may need to be BPF specific simply because the
> > > hook would be dealing with the specific concept of delegating BPF
> > > permissions.
> >
> > Sure. As an alternative, if this is about controlling BPF delegation,
> > instead of doing mount-time checks and LSM hook, perhaps we can add a
> > new LSM hook to BPF_CREATE_TOKEN, just like we have ones for
> > BPF_MAP_CREATE and BPF_PROG_LOAD. That will enable controlling
> > delegation more directly when it is actually attempted to be used.
>
> I'm also going to reply to the v6 patchset, but I thought there were
> some important points in this thread that were worth responding to
> here so that it would have the context of our previous discussion.
>
> So yes, from an LSM perspective we are concerned with who grants the
> delegation (creates the token) and who leverages that token to do
> work.  When this patchset was still using anonymous inodes, marking
> and controlling token creation was relatively easy as we have existing
> hooks/control-points for anonymous inodes which take into account the
> anonymous inode class/type, e.g. bpffs.  Now that this patchset is
> using a regular bpffs inode we may need to do some additional work so
> that we can mark the bpffs token inode as a "token" so that we can
> later distinguish it from an ordinary bpffs inode; it might also serve
> as a convenient place to control creation of the token, but as you
> have already mentioned we could also control this from the existing
> security_bpf(BPF_CREATE_TOKEN, ...) hook at the top of __sys_bpf().
>
> Anyway, more on this in the v6 patchset.
>
> > > I haven't taken the time to write up any hook patches yet as I wanted
> > > to discuss it with you and the others on the To/CC line, but it seems
> > > like we are roughly on the same page, at least with the initial
> > > delegation hook, so I can put something together if you aren't
> > > comfortable working on this (more on this below) ...
> >
> > I'd appreciate the help from the SELinux side specifically, yes. I'm
> > absolutely OK to add a few new LSM hooks, though.
>
> I just want to say again that I'm very happy we can work together to
> make sure everything is covered :)

Likewise! I ran out of time today to finish all the requested changes,
but hopefully I will be able to post a new version tomorrow with all
the feedback applied.

>
> > > > > > As for the bpf_token_allow_xxx(). This feels a bit too specific and
> > > > > > narrow-focused. What if we later add yet another dimension for BPF FS
> > > > > > and token? Do we need to introduce yet another LSM for each such case?
> > > > >
> > > > > [I'm assuming you meant new LSM *hook*]
> > > >
> > > > yep, of course, sorry about using terminology sloppily
> > > >
> > > > > Possibly.  There are also some other issues which I've been thinking
> > > > > about along these lines, specifically the fact that the
> > > > > capability/command delegation happens after the existing
> > > > > security_bpf() hook is called which makes things rather awkward from a
> > > > > LSM perspective: the LSM would first need to allow the process access
> > > > > to the desired BPF op using it's current LSM specific security
> > > > > attributes (e.g. SELinux security domain, etc.) and then later
> > > > > consider the op in the context of the delegated access control rights
> > > > > (if the LSM decides to support those hooks).
> > > > >
> > > > > I suspect that if we want to make this practical we would need to
> > > > > either move some of the token code up into __sys_bpf() so we could
> > > > > have a better interaction with security_bpf(), or we need to consider
> > > > > moving the security_bpf() call into the op specific functions.  I'm
> > > > > still thinking on this (lots of reviews to get through this week), but
> > > > > I'm hoping there is a better way because I'm not sure I like either
> > > > > option very much.
> > > >
> > > > Yes, security_bpf() is happening extremely early and is lacking a lot
> > > > of context. I'm not sure if moving it around is a good idea as it
> > > > basically changes its semantics.
> > >
> > > There are a couple of things that make this not quite as scary as it
> > > may seem.  The first is that currently only SELinux implements a
> > > security_bpf() hook and the implementation is rather simplistic in
> > > terms of what information it requires to perform the existing access
> > > controls; decomposing the single security_bpf() call site into
> > > multiple op specific calls, perhaps with some op specific hooks,
> > > should be doable without causing major semantic changes.  The second
> > > thing is that we could augment the existing security_bpf() hook and
> > > call site with a new LSM hook(s) that are called from the op specific
> > > call sites; this would allow those LSMs that desire the current
> > > semantics to use the existing security_bpf() hook and those that wish
> > > to use the new semantics could implement the new hook(s).  This is
> > > very similar to the pathname-based and inode-based hooks in the VFS
> > > layer, some LSMs choose to implement pathname-based security and use
> > > one set of hooks, while others implement a label-based security
> > > mechanism and use a different set of hooks.
> >
> > Agreed. I think new LSM hooks that are operation-specific make a lot
> > of sense. I'd probably not touch existing security_bpf(), it's an
> > early-entry LSM hook for anything bpf() syscall-specific. This might
> > be very useful in some cases, probably.
> >
> > > > But adding a new set of coherent LSM
> > > > hooks per each appropriate BPF operation with good context to make
> > > > decisions sounds like a good improvement. E.g., for BPF_PROG_LOAD, we
> > > > can have LSM hook after struct bpf_prog is allocated, bpf_token is
> > > > available, attributes are sanity checked. All that together is a very
> > > > useful and powerful context that can be used both by more fixed LSM
> > > > policies (like SELinux), and very dynamic user-defined BPF LSM
> > > > programs.
> > >
> > > This is where it is my turn to mention that I'm getting a bit out of
> > > my depth, but I'm hopeful that the two of us can keep each other from
> > > drowning :)
> > >
> > > Typically the LSM hook call sites end up being in the same general
> > > area as the capability checks, usually just after (we want the normal
> > > Linux discretionary access controls to always come first for the sake
> > > of consistency).  Sticking with that approach it looks like we would
> > > end up with a LSM call in bpf_prog_load() right after bpf_capable()
> > > call, the only gotcha with that is the bpf_prog struct isn't populated
> > > yet, but how important is that when we have the bpf_attr info (honest
> > > question, I don't know the answer to this)?
> >
> > Ok, so I agree in general about having LSM hooks close to capability
> > checks, but at least specifically for BPF_PROG_CREATE, it won't work.
> > This bpf_capable() check you mention. This is just one check. If you
> > look into bpf_prog_load() in kernel/bpf/syscall.c, you'll see that we
> > can also check CAP_PERFMON, CAP_NET_ADMIN, and CAP_SYS_ADMIN, in
> > addition to CAP_BPF, based on various aspects (like program type +
> > subtype).
>
> That's a fair point.
>
> > So for such a complex BPF_PROG_CREATE operation I think we
> > should deviate a bit and place LSM in a logical place that would
> > enable doing LSM enforcement with lots of relevant information, but
> > before doing anything dangerous or expensive.
> >
> > For BPF_PROG_LOAD that place seems to be right before bpf_check(),
> > which is BPF verification ...
>
> > ... Right now we have `security_bpf_prog_alloc(prog->aux);`, which is
> > almost in the ideal place, but provides prog->aux instead of program
> > itself (not sure why), and doesn't provide bpf_attr and bpf_token.
> >
> > So I'm thinking that maybe we get rid of bpf_prog_alloc() in favor of
> > new security_bpf_prog_load(prog, &attr, token)?
>
> That sounds reasonable.  We'll need to make sure we update the docs
> for that LSM hook to indicate that it performs both allocation of the
> LSM's BPF program state (it's current behavior), as well as access
> control for BPF program loads both with and without delegation.

Heh, I asked about this distinction between "allocation of LSM state"
and "access control" in another email thread, but based on this it
seems like it's purely convention based and it's OK to do both with
the same hook, right? If that's the case, then I think we should
combine what you proposed as two hooks, security_bpf_token_alloc() and
security_bpf_token_create(), into one hook to minimize mental
overhead. But if I'm missing something, please point it out on the
patch.

>
> I think those are the big points worth wrapping up here in this
> thread, I'll move the rest over to the v6 patchset.

It all makes sense and I intend to add all those hooks and do
refactoring of existing map/prog ones. I will put that into a separate
patch in the series for a more focused review.

>
> --
> paul-moore.com