lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 23 Jan 2020 11:09:13 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     KP Singh <kpsingh@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for
 the BPF LSM

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@...gle.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@...gle.com>
>>> Reviewed-by: Florent Revest <revest@...gle.com>
>>> Reviewed-by: Thomas Garnier <thgarnie@...gle.com>
>>> Signed-off-by: KP Singh <kpsingh@...gle.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@...r.kernel.org
>>>  L:	bpf@...r.kernel.org
>>>  S:	Maintained
>>>  F:	security/bpf/
>>> +F:	include/linux/bpf_lsm.h
>>>  
>>>  BROADCOM B44 10/100 ETHERNET DRIVER
>>>  M:	Michael Chan <michael.chan@...adcom.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 */

Powered by blists - more mailing lists