[PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
Casey Schaufler
casey at schaufler-ca.com
Thu Jan 23 19:09:13 UTC 2020
On 1/23/2020 9:59 AM, KP Singh wrote:
> On 23-Jan 09:03, Casey Schaufler wrote:
>> On 1/23/2020 7:24 AM, KP Singh wrote:
>>> From: KP Singh <kpsingh at 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 at google.com>
>>> Reviewed-by: Florent Revest <revest at google.com>
>>> Reviewed-by: Thomas Garnier <thgarnie at google.com>
>>> Signed-off-by: KP Singh <kpsingh at 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 at vger.kernel.org
>>> L: bpf at 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 at 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.
> The BPF LSM does not define static hooks (the ones registered to
> security_hook_heads in security.c with __lsm_ro_after_init) for each
> LSM hook. If it tries to do that one ends with what was in v1:
>
> https://lore.kernel.org/bpf/20191220154208.15895-7-kpsingh@chromium.org
>
> This gets quite ugly (security/bpf/hooks.h from v1) and was noted by
> the BPF maintainers:
>
> https://lore.kernel.org/bpf/20191222012722.gdqhppxpfmqfqbld@ast-mbp.dhcp.thefacebook.com/
>
> As I mentioned, some of the ideas we used here are based on:
>
> https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
>
> Which gave each LSM the ability to add mutable hooks at runtime. If
> you prefer we can make this generic and allow the LSMs to register
> mutable hooks with the BPF LSM be the only LSM that uses it (and
> enforce it with a whitelist).
>
> Would this generic approach be something you would consider better
> than just calling the BPF mutable hooks directly?
What I think makes sense is for the BPF LSM to have a hook
for each of the interfaces and for that hook to handle the
mutable list for the interface. If BPF not included there
will be no mutable hooks.
Yes, your v1 got this right.
>
> - KP
>
>>> } 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 */
More information about the Linux-security-module-archive
mailing list