Messages in this thread | | | Subject | Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM | From | Casey Schaufler <> | Date | Thu, 23 Jan 2020 09:03:47 -0800 |
| |
On 1/23/2020 7:24 AM, KP Singh wrote: > From: KP Singh <kpsingh@google.com> > > - The list of hooks registered by an LSM is currently immutable as they > are declared with __lsm_ro_after_init and they are attached to a > security_hook_heads struct. > - For the BPF LSM we need to de/register the hooks at runtime. Making > the existing security_hook_heads mutable broadens an > attack vector, so a separate security_hook_heads is added for only > those that ~must~ be mutable. > - These mutable hooks are run only after all the static hooks have > successfully executed. > > This is based on the ideas discussed in: > > https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal > > Reviewed-by: Brendan Jackman <jackmanb@google.com> > Reviewed-by: Florent Revest <revest@google.com> > Reviewed-by: Thomas Garnier <thgarnie@google.com> > Signed-off-by: KP Singh <kpsingh@google.com> > --- > MAINTAINERS | 1 + > include/linux/bpf_lsm.h | 72 +++++++++++++++++++++++++++++++++++++++++ > security/bpf/Kconfig | 1 + > security/bpf/Makefile | 2 +- > security/bpf/hooks.c | 20 ++++++++++++ > security/bpf/lsm.c | 7 ++++ > security/security.c | 25 +++++++------- > 7 files changed, 116 insertions(+), 12 deletions(-) > create mode 100644 include/linux/bpf_lsm.h > create mode 100644 security/bpf/hooks.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e2b7f76a1a70..c606b3d89992 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3209,6 +3209,7 @@ L: linux-security-module@vger.kernel.org > L: bpf@vger.kernel.org > S: Maintained > F: security/bpf/ > +F: include/linux/bpf_lsm.h > > BROADCOM B44 10/100 ETHERNET DRIVER > M: Michael Chan <michael.chan@broadcom.com> > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h > new file mode 100644 > index 000000000000..57c20b2cd2f4 > --- /dev/null > +++ b/include/linux/bpf_lsm.h > @@ -0,0 +1,72 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* > + * Copyright 2019 Google LLC. > + */ > + > +#ifndef _LINUX_BPF_LSM_H > +#define _LINUX_BPF_LSM_H > + > +#include <linux/bpf.h> > +#include <linux/lsm_hooks.h> > + > +#ifdef CONFIG_SECURITY_BPF > + > +/* Mutable hooks defined at runtime and executed after all the statically > + * defined LSM hooks. > + */ > +extern struct security_hook_heads bpf_lsm_hook_heads; > + > +int bpf_lsm_srcu_read_lock(void); > +void bpf_lsm_srcu_read_unlock(int idx); > + > +#define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...) \ > + do { \ > + struct security_hook_list *P; \ > + int _idx; \ > + \ > + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \ > + break; \ > + \ > + _idx = bpf_lsm_srcu_read_lock(); \ > + hlist_for_each_entry(P, &bpf_lsm_hook_heads.FUNC, list) \ > + P->hook.FUNC(__VA_ARGS__); \ > + bpf_lsm_srcu_read_unlock(_idx); \ > + } while (0) > + > +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) ({ \ > + int _ret = 0; \ > + do { \ > + struct security_hook_list *P; \ > + int _idx; \ > + \ > + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \ > + break; \ > + \ > + _idx = bpf_lsm_srcu_read_lock(); \ > + \ > + hlist_for_each_entry(P, \ > + &bpf_lsm_hook_heads.FUNC, list) { \ > + _ret = P->hook.FUNC(__VA_ARGS__); \ > + if (_ret && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \ > + break; \ > + } \ > + bpf_lsm_srcu_read_unlock(_idx); \ > + } while (0); \ > + IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? _ret : 0; \ > +}) > + > +#else /* !CONFIG_SECURITY_BPF */ > + > +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) (0) > +#define CALL_BPF_LSM_VOID_HOOKS(...) > + > +static inline int bpf_lsm_srcu_read_lock(void) > +{ > + return 0; > +} > +static inline void bpf_lsm_srcu_read_unlock(int idx) {} > + > +#endif /* CONFIG_SECURITY_BPF */ > + > +#endif /* _LINUX_BPF_LSM_H */ > diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig > index a5f6c67ae526..595e4ad597ae 100644 > --- a/security/bpf/Kconfig > +++ b/security/bpf/Kconfig > @@ -6,6 +6,7 @@ config SECURITY_BPF > bool "BPF-based MAC and audit policy" > depends on SECURITY > depends on BPF_SYSCALL > + depends on SRCU > help > This enables instrumentation of the security hooks with > eBPF programs. > diff --git a/security/bpf/Makefile b/security/bpf/Makefile > index c78a8a056e7e..c526927c337d 100644 > --- a/security/bpf/Makefile > +++ b/security/bpf/Makefile > @@ -2,4 +2,4 @@ > # > # Copyright 2019 Google LLC. > > -obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o > +obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c > new file mode 100644 > index 000000000000..b123d9cb4cd4 > --- /dev/null > +++ b/security/bpf/hooks.c > @@ -0,0 +1,20 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Copyright 2019 Google LLC. > + */ > + > +#include <linux/bpf_lsm.h> > +#include <linux/srcu.h> > + > +DEFINE_STATIC_SRCU(security_hook_srcu); > + > +int bpf_lsm_srcu_read_lock(void) > +{ > + return srcu_read_lock(&security_hook_srcu); > +} > + > +void bpf_lsm_srcu_read_unlock(int idx) > +{ > + return srcu_read_unlock(&security_hook_srcu, idx); > +} > diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c > index dc9ac03c7aa0..a25a068e1781 100644 > --- a/security/bpf/lsm.c > +++ b/security/bpf/lsm.c > @@ -4,6 +4,7 @@ > * Copyright 2019 Google LLC. > */ > > +#include <linux/bpf_lsm.h> > #include <linux/lsm_hooks.h> > > /* This is only for internal hooks, always statically shipped as part of the > @@ -12,6 +13,12 @@ > */ > static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {}; > > +/* Security hooks registered dynamically by the BPF LSM and must be accessed > + * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock. The mutable > + * hooks dynamically allocated by the BPF LSM are appeneded here. > + */ > +struct security_hook_heads bpf_lsm_hook_heads; > + > static int __init bpf_lsm_init(void) > { > security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf"); > diff --git a/security/security.c b/security/security.c > index 30a8aa700557..95a46ca25dcd 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -27,6 +27,7 @@ > #include <linux/backing-dev.h> > #include <linux/string.h> > #include <linux/msg.h> > +#include <linux/bpf_lsm.h> > #include <net/flow.h> > > #define MAX_LSM_EVM_XATTR 2 > @@ -657,20 +658,22 @@ static void __init lsm_early_task(struct task_struct *task) > \ > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \ > P->hook.FUNC(__VA_ARGS__); \ > + CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__); \
I'm sorry if I wasn't clear on the v2 review. This does not belong in the infrastructure. You should be doing all the bpf_lsm hook processing in you module. bpf_lsm_task_alloc() should loop though all the bpf task_alloc hooks if they have to be handled differently from "normal" LSM hooks.
> } while (0) > > -#define call_int_hook(FUNC, IRC, ...) ({ \ > - int RC = IRC; \ > - do { \ > - struct security_hook_list *P; \ > - \ > +#define call_int_hook(FUNC, IRC, ...) ({ \ > + int RC = IRC; \ > + do { \ > + struct security_hook_list *P; \ > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > - RC = P->hook.FUNC(__VA_ARGS__); \ > - if (RC != 0) \ > - break; \ > - } \ > - } while (0); \ > - RC; \ > + RC = P->hook.FUNC(__VA_ARGS__); \ > + if (RC != 0) \ > + break; \ > + } \ > + if (RC == 0) \ > + RC = CALL_BPF_LSM_INT_HOOKS(FUNC, __VA_ARGS__); \ > + } while (0); \ > + RC; \ > }) > > /* Security operations */
| |