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

[Andrii Nakryiko]

On Sun, Nov 5, 2023 at 9:01 PM Paul Moore <paul at paul-moore.com> wrote:
>
> On Nov  3, 2023 Andrii Nakryiko <andrii at 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 at 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.

Makes sense, renamed to security_bpf_token_cmd() and updated hook name as well

>
> > 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(...))
>   }

yep, makes sense, I changed it as you suggested above

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

it would have to be !security_bpf_token_cmd(), right? And that single
negation is just very confusing when dealing with int-returning
function. I find it much easier to make sure the logic is correct when
we have explicit `== 0`.

Like, when I see `return !security_bpf_token_cmd(...);`, my immediate
read of that is "return whether bpf_token_cmd is not allowed" or
something along those lines, giving me a huge pause... I have the same
relationship with strcmp(), btw, while people seem totally fine with
`!strcmp()` (which to me also reads backwards).

Anyways, unless you really feel strongly, I'd keep == 0 here and above
for security_bpf_token_capable(), just because it's int-returning
function result conversion to bool-returning result.

>
> >  }
> >
> >  bool bpf_token_allow_map_type(const struct bpf_token *token, enum bpf_map_type type)
>
> --
> paul-moore.com