Re: [PATCH v9 11/17] bpf,lsm: add BPF token LSM hooks

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

On Nov  3, 2023 Andrii Nakryiko <andrii@kernel.org> wrote:
> 
> Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to
> allocate LSM security blob (we add `void *security` field to struct
> bpf_token for that), but also control who can instantiate BPF token.
> This follows existing pattern for BPF map and BPF prog.
> 
> Also add security_bpf_token_allow_cmd() and security_bpf_token_capable()
> LSM hooks that allow LSM implementation to control and negate (if
> necessary) BPF token's delegation of a specific bpf_cmd and capability,
> respectively.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  include/linux/bpf.h           |  3 ++
>  include/linux/lsm_hook_defs.h |  5 +++
>  include/linux/security.h      | 25 +++++++++++++++
>  kernel/bpf/bpf_lsm.c          |  4 +++
>  kernel/bpf/token.c            | 13 ++++++--
>  security/security.c           | 60 +++++++++++++++++++++++++++++++++++
>  6 files changed, 107 insertions(+), 3 deletions(-)

...

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 08fd777cbe94..1d6edbf45d1c 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -60,6 +60,7 @@ struct fs_parameter;
>  enum fs_value_type;
>  struct watch;
>  struct watch_notification;
> +enum bpf_cmd;

Yes, I think it's fine to include bpf.h in security.h instead of the
forward declaration.

>  /* Default (no) options for the capable function */
>  #define CAP_OPT_NONE 0x0
> @@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map);
>  extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
>  				  struct bpf_token *token);
>  extern void security_bpf_prog_free(struct bpf_prog *prog);
> +extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr,
> +				     struct path *path);
> +extern void security_bpf_token_free(struct bpf_token *token);
> +extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd);
> +extern int security_bpf_token_capable(const struct bpf_token *token, int cap);
>  #else
>  static inline int security_bpf(int cmd, union bpf_attr *attr,
>  					     unsigned int size)
> @@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *
>  
>  static inline void security_bpf_prog_free(struct bpf_prog *prog)
>  { }
> +
> +static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr,
> +				     struct path *path)
> +{
> +	return 0;
> +}
> +
> +static inline void security_bpf_token_free(struct bpf_token *token)
> +{ }
> +
> +static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
> +{
> +	return 0;
> +}
> +
> +static inline int security_bpf_token_capable(const struct bpf_token *token, int cap)
> +{
> +	return 0;
> +}

Another nitpick, but I would prefer to shorten
security_bpf_token_allow_cmd() renamed to security_bpf_token_cmd() both
to shorten the name and to better fit convention.  I realize the caller
is named bpf_token_allow_cmd() but I'd still rather see the LSM hook
with the shorter name.

> diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
> index 35e6f55c2a41..5d04da54faea 100644
> --- a/kernel/bpf/token.c
> +++ b/kernel/bpf/token.c
> @@ -7,11 +7,12 @@
>  #include <linux/idr.h>
>  #include <linux/namei.h>
>  #include <linux/user_namespace.h>
> +#include <linux/security.h>
>  
>  bool bpf_token_capable(const struct bpf_token *token, int cap)
>  {
>  	/* BPF token allows ns_capable() level of capabilities */
> -	if (token) {
> +	if (token && security_bpf_token_capable(token, cap) == 0) {
>  		if (ns_capable(token->userns, cap))
>  			return true;
>  		if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN))

We typically perform the capability based access controls prior to the
LSM controls, meaning if we want to the token controls to work in a
similar way we should do something like this:

  bool bpf_token_capable(...)
  {
    if (token) {
      if (ns_capable(token, cap) ||
          (cap != ADMIN && ns_capable(token, ADMIN)))
        return security_bpf_token_capable(token, cap);
    }
    return capable(cap) || (cap != ADMIN && capable(...))
  }

> @@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token)
>  
>  static void bpf_token_free(struct bpf_token *token)
>  {
> +	security_bpf_token_free(token);
>  	put_user_ns(token->userns);
>  	kvfree(token);
>  }
> @@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr)
>  	token->allowed_progs = mnt_opts->delegate_progs;
>  	token->allowed_attachs = mnt_opts->delegate_attachs;
>  
> +	err = security_bpf_token_create(token, attr, &path);
> +	if (err)
> +		goto out_token;
> +
>  	fd = get_unused_fd_flags(O_CLOEXEC);
>  	if (fd < 0) {
>  		err = fd;
> @@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
>  {
>  	if (!token)
>  		return false;
> -
> -	return token->allowed_cmds & (1ULL << cmd);
> +	if (!(token->allowed_cmds & (1ULL << cmd)))
> +		return false;
> +	return security_bpf_token_allow_cmd(token, cmd) == 0;

I'm not sure how much it really matters, but someone might prefer
the '!!' approach/style over '== 0'.

>  }
>  
>  bool bpf_token_allow_map_type(const struct bpf_token *token, enum bpf_map_type type)

--
paul-moore.com