On Mon, Nov 6, 2023 at 2:17 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
> On Sun, Nov 5, 2023 at 9:01 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Nov 3, 2023 Andrii Nakryiko <andrii@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
Thanks.
> > > 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
Thanks again.
> > > @@ -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?
Yeah :P
In most, although definitely not all, kernel functions when something
returns 0 we consider that the positive/success case, with non-zero
values being some sort of failure. I must have defaulted to that
logic here, but you are correct that just a single negation would be
needed here.
> 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`.
That's fine, it's something I've seen mentioned over the years and
thought I might offer it as a comment. I can read either approach
just fine :)
Anyway, with the other changes mentioned above, e.g. naming and
permission ordering, feel free to add my ACK.
Acked-by: Paul Moore <paul@xxxxxxxxxxxxxx>
--
paul-moore.com
