Re: [PATCH bpf-next v5 5/7] bpf: lsm: Initialize the BPF LSM hooks

[KP Singh <kpsingh@chromium.org>]

On 23-Mär 12:44, Kees Cook wrote:
> On Mon, Mar 23, 2020 at 05:44:13PM +0100, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> > 
> > The bpf_lsm_ nops are initialized into the LSM framework like any other
> > LSM.  Some LSM hooks do not have 0 as their default return value. The
> > __weak symbol for these hooks is overridden by a corresponding
> > definition in security/bpf/hooks.c
> > 
> > The LSM can be enabled / disabled with CONFIG_LSM.
> > 
> > Signed-off-by: KP Singh <kpsingh@google.com>
> 
> Nice! This is super clean on the LSM side of things. :)
> 
> One note below...
> 
> > Reviewed-by: Brendan Jackman <jackmanb@google.com>

[...]

> > +
> > +/*
> > + * Copyright (C) 2020 Google LLC.
> > + */
> > +#include <linux/lsm_hooks.h>
> > +#include <linux/bpf_lsm.h>
> > +
> > +/* Some LSM hooks do not have 0 as their default return values. Override the
> > + * __weak definitons generated by default for these hooks
> 
> If you wanted to avoid this, couldn't you make the default return value
> part of lsm_hooks.h?
> 
> e.g.:
> 
> LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct inode *inode,
> 	 const char *name, void **buffer, bool alloc)
> 
> ...
> 
> #define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
> 	LSM_HOOK_##RET(NAME, DEFAULT, __VA_ARGS__)
> ...
> #define LSM_HOOK_int(NAME, DEFAULT, ...)	\
> noinline int bpf_lsm_##NAME(__VA_ARGS__)	\
> {						\
> 	return (DEFAULT);			\
> }
> 
> Then all the __weak stuff is gone, and the following 4 functions don't
> need to be written out, and the information is available to the macros
> if anyone else might ever want it.

Thanks, I like it!

If no-one objects, I will update it in the next revision.

- KP

> 
> -Kees
> 
> > + */
> > +noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const char *name,
> > +				       void **buffer, bool alloc)
> > +};

[...]

> > -- 
> > 2.20.1
> > 
> 
> -- 
> Kees Cook