Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: KP Singh <kpsingh@chromium.org>
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: KP Singh <kpsingh@chromium.org>,
	bpf@vger.kernel.org,
	Linux Security Module list 
	<linux-security-module@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
Date: Thu, 16 Jan 2020 11:19:40 +0100
Message-ID: <20200116101940.GC240584@google.com> (raw)
In-Reply-To: <5793e9a8-e9cf-dd2d-261d-61f533cca20c@schaufler-ca.com>

On 15-Jan 22:33, Casey Schaufler wrote:
> On 1/15/2020 9:13 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
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  MAINTAINERS             |  1 +
> >  include/linux/bpf_lsm.h | 71 +++++++++++++++++++++++++++++++++++++++++
> >  security/bpf/Kconfig    |  1 +
> >  security/bpf/Makefile   |  2 +-
> >  security/bpf/hooks.c    | 20 ++++++++++++
> >  security/bpf/lsm.c      |  9 +++++-
> >  security/security.c     | 24 +++++++-------
> >  7 files changed, 115 insertions(+), 13 deletions(-)
> >  create mode 100644 include/linux/bpf_lsm.h
> >  create mode 100644 security/bpf/hooks.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0941f478cfa5..02d7e05e9b75 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..9883cf25241c
> > --- /dev/null
> > +++ b/include/linux/bpf_lsm.h
> > @@ -0,0 +1,71 @@
> > +/* 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
> > + * define 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(RC, 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) {	\
> > +			RC = P->hook.FUNC(__VA_ARGS__);		\
> > +			if (RC && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> > +				break;				\
> > +		}						\
> > +		bpf_lsm_srcu_read_unlock(_idx);			\
> > +	} while (0);						\
> > +	IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? RC : 0;	\
> > +})
> > +
> > +#else /* !CONFIG_SECURITY_BPF */
> > +
> > +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) (RC)
> > +#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 5c5c14f990ce..d4ea6aa9ddb8 100644
> > --- a/security/bpf/lsm.c
> > +++ b/security/bpf/lsm.c
> > @@ -4,14 +4,21 @@
> >   * 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
> > - * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> > + * BPF LSM. Statically defined hooks are appended to the security_hook_heads
> >   * which is common for LSMs and R/O after init.
> >   */
> >  static struct security_hook_list 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 lsm_init(void)
> >  {
> >  	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> > diff --git a/security/security.c b/security/security.c
> > index cd2d18d2d279..4a2eb4c089b2 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
> > @@ -652,20 +653,21 @@ 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__);	\
> >  	} 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;					\
> > +		}							\
> > +		RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__);	\
> 
> Do not do this. Add LSM_ORDER_LAST for the lsm_order field of lsm_info
> and use that to identify your module as one to be put on the list last.
> Update the LSM registration code to do this. It will be much like the code
> that uses LSM_ORDER_FIRST to put the capabilities at the head of the lists.
> 
> What you have here to way to much like the way Yama was invoked before
> stacking.

Thanks for taking a look!

The BPF LSM has two kinds of hooks. The static hooks which are
appended to the security_hook_heads in security.c and mutable hooks
which are allocated at runtime and attached to a separate
security_hook_heads (bpf_lsm_hook_heads) defined in security/bpf/lsm.c
This macro runs these mutable hooks (dynamically allocated at runtime)
under SRCU protection.

Having a separate security_hook_heads allows:

- The security_hook_heads in security.c to be __lsm_ro_after_init
  and thus limits the attack surface does not change the existing
  behaviour
- The SRCU critical section be limited to the execution of dynamically
  allocated hooks

I agree that the static hooks should indeed be LSM_ORDER_LAST which
makes logical sense and represents correctly how the LSM will behave
in reality. (i.e. its hooks are executed last irrespective of the
position it is mentioned in the list of LSMs.)

I will update this for v3.

- KP

> 
> > +	} while (0);							\
> > +	RC;								\
> >  })
> >  
> >  /* Security operations */
> 

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-15 17:13 [PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI) KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 01/10] bpf: btf: Make some of the API visible outside BTF KP Singh
2020-01-18 12:44   ` kbuild test robot
2020-01-15 17:13 ` [PATCH bpf-next v2 02/10] bpf: lsm: Add a skeleton and config options KP Singh
2020-01-16  7:04   ` Casey Schaufler
2020-01-16 12:52     ` KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 03/10] bpf: lsm: Introduce types for eBPF based LSM KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM KP Singh
2020-01-15 17:30   ` Stephen Smalley
2020-01-16  9:48     ` KP Singh
2020-01-16  6:33   ` Casey Schaufler
2020-01-16 10:19     ` KP Singh [this message]
2020-01-15 17:13 ` [PATCH bpf-next v2 05/10] bpf: lsm: BTF API for LSM hooks KP Singh
2020-01-17  0:28   ` Andrii Nakryiko
2020-01-15 17:13 ` [PATCH bpf-next v2 06/10] bpf: lsm: Implement attach, detach and execution KP Singh
2020-01-15 17:24   ` Greg Kroah-Hartman
2020-01-16  9:45     ` KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 07/10] bpf: lsm: Make the allocated callback RO+X KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM KP Singh
2020-01-15 21:19   ` Andrii Nakryiko
2020-01-15 21:37     ` Andrii Nakryiko
2020-01-16 12:49     ` KP Singh
2020-01-16 17:26       ` KP Singh
2020-01-16 19:10       ` Andrii Nakryiko
2020-01-17 22:16         ` KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 09/10] bpf: lsm: Add selftests " KP Singh
2020-01-15 17:13 ` [PATCH bpf-next v2 10/10] bpf: lsm: Add Documentation KP Singh
2020-01-15 22:12 ` [PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI) Andrii Nakryiko
2020-01-16 10:03 ` Brendan Jackman

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200116101940.GC240584@google.com \
    --to=kpsingh@chromium.org \
    --cc=bpf@vger.kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=linux-security-module@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git