Date: Mon, 6 Nov 2023 17:29:40 -0500
From: Paul Moore <paul@...l-moore.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	brauner@...nel.org, linux-fsdevel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, keescook@...omium.org, 
	kernel-team@...a.com, sargun@...gun.me
Subject: Re: [PATCH v9 9/17] bpf,lsm: refactor bpf_prog_alloc/bpf_prog_free
 LSM hooks

On Mon, Nov 6, 2023 at 2:03 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
> On Sun, Nov 5, 2023 at 9:01 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Nov  3, 2023 Andrii Nakryiko <andrii@...nel.org> wrote:
> > >
> > > Based on upstream discussion ([0]), rework existing
> > > bpf_prog_alloc_security LSM hook. Rename it to bpf_prog_load and instead
> > > of passing bpf_prog_aux, pass proper bpf_prog pointer for a full BPF
> > > program struct. Also, we pass bpf_attr union with all the user-provided
> > > arguments for BPF_PROG_LOAD command.  This will give LSMs as much
> > > information as we can basically provide.
> > >
> > > The hook is also BPF token-aware now, and optional bpf_token struct is
> > > passed as a third argument. bpf_prog_load LSM hook is called after
> > > a bunch of sanity checks were performed, bpf_prog and bpf_prog_aux were
> > > allocated and filled out, but right before performing full-fledged BPF
> > > verification step.
> > >
> > > bpf_prog_free LSM hook is now accepting struct bpf_prog argument, for
> > > consistency. SELinux code is adjusted to all new names, types, and
> > > signatures.
> > >
> > > Note, given that bpf_prog_load (previously bpf_prog_alloc) hook can be
> > > used by some LSMs to allocate extra security blob, but also by other
> > > LSMs to reject BPF program loading, we need to make sure that
> > > bpf_prog_free LSM hook is called after bpf_prog_load/bpf_prog_alloc one
> > > *even* if the hook itself returned error. If we don't do that, we run
> > > the risk of leaking memory. This seems to be possible today when
> > > combining SELinux and BPF LSM, as one example, depending on their
> > > relative ordering.
> > >
> > > Also, for BPF LSM setup, add bpf_prog_load and bpf_prog_free to
> > > sleepable LSM hooks list, as they are both executed in sleepable
> > > context. Also drop bpf_prog_load hook from untrusted, as there is no
> > > issue with refcount or anything else anymore, that originally forced us
> > > to add it to untrusted list in c0c852dd1876 ("bpf: Do not mark certain LSM
> > > hook arguments as trusted"). We now trigger this hook much later and it
> > > should not be an issue anymore.
> >
> > See my comment below, but it isn't clear to me if this means it is okay
> > to have `BTF_ID(func, bpf_lsm_bpf_prog_free)` called twice.  It probably
> > would be a good idea to get KP, BPF LSM maintainer, to review this change
> > as well to make sure this looks good to him.
> >
> > >   [0] https://lore.kernel.org/bpf/9fe88aef7deabbe87d3fc38c4aea3c69.paul@paul-moore.com/
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> > > ---
> > >  include/linux/lsm_hook_defs.h |  5 +++--
> > >  include/linux/security.h      | 12 +++++++-----
> > >  kernel/bpf/bpf_lsm.c          |  5 +++--
> > >  kernel/bpf/syscall.c          | 25 +++++++++++++------------
> > >  security/security.c           | 25 +++++++++++++++----------
> > >  security/selinux/hooks.c      | 15 ++++++++-------
> > >  6 files changed, 49 insertions(+), 38 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > index e14c822f8911..3e956f6302f3 100644
> > > --- a/kernel/bpf/bpf_lsm.c
> > > +++ b/kernel/bpf/bpf_lsm.c
> > > @@ -263,6 +263,8 @@ BTF_ID(func, bpf_lsm_bpf_map)
> > >  BTF_ID(func, bpf_lsm_bpf_map_alloc_security)
> > >  BTF_ID(func, bpf_lsm_bpf_map_free_security)
> > >  BTF_ID(func, bpf_lsm_bpf_prog)
> > > +BTF_ID(func, bpf_lsm_bpf_prog_load)
> > > +BTF_ID(func, bpf_lsm_bpf_prog_free)
> > >  BTF_ID(func, bpf_lsm_bprm_check_security)
> > >  BTF_ID(func, bpf_lsm_bprm_committed_creds)
> > >  BTF_ID(func, bpf_lsm_bprm_committing_creds)
> > > @@ -346,8 +348,7 @@ BTF_SET_END(sleepable_lsm_hooks)
> > >
> > >  BTF_SET_START(untrusted_lsm_hooks)
> > >  BTF_ID(func, bpf_lsm_bpf_map_free_security)
> > > -BTF_ID(func, bpf_lsm_bpf_prog_alloc_security)
> > > -BTF_ID(func, bpf_lsm_bpf_prog_free_security)
> > > +BTF_ID(func, bpf_lsm_bpf_prog_free)
> > >  BTF_ID(func, bpf_lsm_file_alloc_security)
> > >  BTF_ID(func, bpf_lsm_file_free_security)
> > >  #ifdef CONFIG_SECURITY_NETWORK
> >
> > It looks like you're calling the BTF_ID() macro on bpf_lsm_bpf_prog_free
> > twice?  I would have expected a only one macro call for each bpf_prog_load
> > and bpf_prog_free, is that a bad assuption?
> >
>
> Yeah, there is no problem having multiple BTF_ID() invocations for the
> same function. BTF_ID() macro (conceptually) emits a relocation that
> will instruct resolve_btfids tool to put an actual BTF ID for the
> specified function in a designated 4-byte slot.
>
> In this case, we have two separate lists: sleepable_lsm_hooks and
> untrusted_lsm_hooks, so we need two separate BTF_ID() entries for the
> same function. It's expected to be duplicated.

Okay, thanks for the explanation.  It jumped out as a deviation from
the current code when I was looking at the changes and I was worried
that it was a typo, but it sounds like it's expected and the proper
thing to do.

> > > diff --git a/security/security.c b/security/security.c
> > > index dcb3e7014f9b..5773d446210e 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -5180,16 +5180,21 @@ int security_bpf_map_alloc(struct bpf_map *map)
> > >  }
> > >
> > >  /**
> > > - * security_bpf_prog_alloc() - Allocate a bpf program LSM blob
> > > - * @aux: bpf program aux info struct
> > > + * security_bpf_prog_load() - Check if loading of BPF program is allowed
> > > + * @prog: BPF program object
> > > + * @attr: BPF syscall attributes used to create BPF program
> > > + * @token: BPF token used to grant user access to BPF subsystem
> > >   *
> > > - * Initialize the security field inside bpf program.
> > > + * Do a check when the kernel allocates BPF program object and is about to
> > > + * pass it to BPF verifier for additional correctness checks. This is also the
> > > + * point where LSM blob is allocated for LSMs that need them.
> >
> > This is pretty nitpicky, but I'm guessing you may need to make another
> > revision to this patchset, if you do please drop the BPF verifier remark
> > from the comment above.
> >
> > Example: "Perform an access control check when the kernel loads a BPF
> > program and allocates the associated BPF program object.  This hook is
> > also responsibile for allocating any required LSM state for the BPF
> > program."
>
> Done, no problem.

With the comment change above, and the clarification of the BTF_ID()
calls, feel free to add my ACK.

Acked-by: Paul Moore <paul@...l-moore.com>

-- 
paul-moore.com

Powered by blists - more mailing lists