Date: Mon, 6 Nov 2023 11:03:05 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Paul Moore <paul@...l-moore.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 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.

> > 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.

>
> >   * Return: Returns 0 on success, error on failure.
> >   */
> > -int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
> > +int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
> > +                        struct bpf_token *token)
> >  {
> > -     return call_int_hook(bpf_prog_alloc_security, 0, aux);
> > +     return call_int_hook(bpf_prog_load, 0, prog, attr, token);
> >  }
>
> --
> paul-moore.com

Powered by blists - more mailing lists