Re: [PATCH v9 9/17] bpf,lsm: refactor bpf_prog_alloc/bpf_prog_free LSM hooks

[Paul Moore <paul@paul-moore.com>]

On Nov  3, 2023 Andrii Nakryiko <andrii@kernel.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@kernel.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?

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

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