diff mbox series

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

Message ID 20200323164415.12943-6-kpsingh@chromium.org
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series MAC and Audit policy using eBPF (KRSI) | expand

Commit Message

KP Singh March 23, 2020, 4:44 p.m. UTC 
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>
Reviewed-by: Brendan Jackman <jackmanb@google.com>
Reviewed-by: Florent Revest <revest@google.com>
---
 security/Kconfig      | 10 ++++----
 security/Makefile     |  2 ++
 security/bpf/Makefile |  5 ++++
 security/bpf/hooks.c  | 55 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100644 security/bpf/Makefile
 create mode 100644 security/bpf/hooks.c

Comments

Kees Cook March 23, 2020, 7:44 p.m. UTC | #1 
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>
> Reviewed-by: Florent Revest <revest@google.com>
> ---
>  security/Kconfig      | 10 ++++----
>  security/Makefile     |  2 ++
>  security/bpf/Makefile |  5 ++++
>  security/bpf/hooks.c  | 55 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 67 insertions(+), 5 deletions(-)
>  create mode 100644 security/bpf/Makefile
>  create mode 100644 security/bpf/hooks.c
> 
> diff --git a/security/Kconfig b/security/Kconfig
> index 2a1a2d396228..cd3cc7da3a55 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -277,11 +277,11 @@ endchoice
>  
>  config LSM
>  	string "Ordered list of enabled LSMs"
> -	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
> -	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
> -	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> -	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
> -	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> +	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> +	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> +	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> +	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
> +	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
>  	help
>  	  A comma-separated list of LSMs, in initialization order.
>  	  Any LSMs left off this list will be ignored. This can be
> diff --git a/security/Makefile b/security/Makefile
> index 746438499029..22e73a3482bd 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA)		+= yama
>  subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
>  subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
>  subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
> +subdir-$(CONFIG_BPF_LSM)		+= bpf
>  
>  # always enable default capabilities
>  obj-y					+= commoncap.o
> @@ -30,6 +31,7 @@ obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
>  obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
>  obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
>  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
> +obj-$(CONFIG_BPF_LSM)			+= bpf/
>  
>  # Object integrity file lists
>  subdir-$(CONFIG_INTEGRITY)		+= integrity
> diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> new file mode 100644
> index 000000000000..c7a89a962084
> --- /dev/null
> +++ b/security/bpf/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2020 Google LLC.
> +
> +obj-$(CONFIG_BPF_LSM) := hooks.o
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> new file mode 100644
> index 000000000000..68e5824868f9
> --- /dev/null
> +++ b/security/bpf/hooks.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * 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.

-Kees

> + */
> +noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const char *name,
> +				       void **buffer, bool alloc)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +noinline int bpf_lsm_inode_setsecurity(struct inode *inode, const char *name,
> +				       const void *value, size_t size,
> +				       int flags)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +noinline int bpf_lsm_task_prctl(int option, unsigned long arg2,
> +				unsigned long arg3, unsigned long arg4,
> +				unsigned long arg5)
> +{
> +	return -ENOSYS;
> +}
> +
> +noinline int bpf_lsm_xfrm_state_pol_flow_match(struct xfrm_state *x,
> +					       struct xfrm_policy *xp,
> +					       const struct flowi *fl)
> +{
> +	return 1;
> +}
> +
> +static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
> +	#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
> +	#include <linux/lsm_hook_names.h>
> +	#undef LSM_HOOK
> +};
> +
> +static int __init bpf_lsm_init(void)
> +{
> +	security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
> +	pr_info("LSM support for eBPF active\n");
> +	return 0;
> +}
> +
> +DEFINE_LSM(bpf) = {
> +	.name = "bpf",
> +	.init = bpf_lsm_init,
> +};
> -- 
> 2.20.1
>
KP Singh March 23, 2020, 7:47 p.m. UTC | #2 
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
Andrii Nakryiko March 23, 2020, 8:21 p.m. UTC | #3 
On Mon, Mar 23, 2020 at 12:48 PM KP Singh <kpsingh@chromium.org> wrote:
>
> 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.
>

I was about to propose the same, explicit default value seems like a
much cleaner and more straightforward way to do this.

> - KP
>
> >
> > -Kees
> >
> > > + */
> > > +noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const char *name,
> > > +                                  void **buffer, bool alloc)
> > > +};
>
> [...]
>
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Kees Cook
Casey Schaufler March 23, 2020, 8:47 p.m. UTC | #4 
On 3/23/2020 12:44 PM, 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>
>> Reviewed-by: Florent Revest <revest@google.com>
>> ---
>>  security/Kconfig      | 10 ++++----
>>  security/Makefile     |  2 ++
>>  security/bpf/Makefile |  5 ++++
>>  security/bpf/hooks.c  | 55 +++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 67 insertions(+), 5 deletions(-)
>>  create mode 100644 security/bpf/Makefile
>>  create mode 100644 security/bpf/hooks.c
>>
>> diff --git a/security/Kconfig b/security/Kconfig
>> index 2a1a2d396228..cd3cc7da3a55 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -277,11 +277,11 @@ endchoice
>>  
>>  config LSM
>>  	string "Ordered list of enabled LSMs"
>> -	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
>> -	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
>> -	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
>> -	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
>> -	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
>> +	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>> +	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>> +	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>> +	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
>> +	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
>>  	help
>>  	  A comma-separated list of LSMs, in initialization order.
>>  	  Any LSMs left off this list will be ignored. This can be
>> diff --git a/security/Makefile b/security/Makefile
>> index 746438499029..22e73a3482bd 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA)		+= yama
>>  subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
>>  subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
>>  subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
>> +subdir-$(CONFIG_BPF_LSM)		+= bpf
>>  
>>  # always enable default capabilities
>>  obj-y					+= commoncap.o
>> @@ -30,6 +31,7 @@ obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
>>  obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
>>  obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
>>  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
>> +obj-$(CONFIG_BPF_LSM)			+= bpf/
>>  
>>  # Object integrity file lists
>>  subdir-$(CONFIG_INTEGRITY)		+= integrity
>> diff --git a/security/bpf/Makefile b/security/bpf/Makefile
>> new file mode 100644
>> index 000000000000..c7a89a962084
>> --- /dev/null
>> +++ b/security/bpf/Makefile
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Copyright (C) 2020 Google LLC.
>> +
>> +obj-$(CONFIG_BPF_LSM) := hooks.o
>> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
>> new file mode 100644
>> index 000000000000..68e5824868f9
>> --- /dev/null
>> +++ b/security/bpf/hooks.c
>> @@ -0,0 +1,55 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * 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)

If you're going to do that you'll have to keep lsm_hooks.h and security.c
default values in sync somehow. Note that the four functions you've called
out won't be using call_int_hook() after the next round of stacking. I'm not
nixing the idea, I just don't want the default return for the security_
functions defined in two places.

>
> ...
>
> #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.
>
> -Kees
>
>> + */
>> +noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const char *name,
>> +				       void **buffer, bool alloc)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +noinline int bpf_lsm_inode_setsecurity(struct inode *inode, const char *name,
>> +				       const void *value, size_t size,
>> +				       int flags)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +noinline int bpf_lsm_task_prctl(int option, unsigned long arg2,
>> +				unsigned long arg3, unsigned long arg4,
>> +				unsigned long arg5)
>> +{
>> +	return -ENOSYS;
>> +}
>> +
>> +noinline int bpf_lsm_xfrm_state_pol_flow_match(struct xfrm_state *x,
>> +					       struct xfrm_policy *xp,
>> +					       const struct flowi *fl)
>> +{
>> +	return 1;
>> +}
>> +
>> +static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
>> +	#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
>> +	#include <linux/lsm_hook_names.h>
>> +	#undef LSM_HOOK
>> +};
>> +
>> +static int __init bpf_lsm_init(void)
>> +{
>> +	security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
>> +	pr_info("LSM support for eBPF active\n");
>> +	return 0;
>> +}
>> +
>> +DEFINE_LSM(bpf) = {
>> +	.name = "bpf",
>> +	.init = bpf_lsm_init,
>> +};
>> -- 
>> 2.20.1
>>
Kees Cook March 23, 2020, 9:44 p.m. UTC | #5 
On Mon, Mar 23, 2020 at 01:47:29PM -0700, Casey Schaufler wrote:
> On 3/23/2020 12:44 PM, Kees Cook wrote:
> > On Mon, Mar 23, 2020 at 05:44:13PM +0100, KP Singh wrote:
> >> +/* 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)
> 
> If you're going to do that you'll have to keep lsm_hooks.h and security.c
> default values in sync somehow. Note that the four functions you've called
> out won't be using call_int_hook() after the next round of stacking. I'm not
> nixing the idea, I just don't want the default return for the security_
> functions defined in two places.

Yeah, I actually went looking for this after I sent the email, realizing
that the defaults were also used in security.c. I've been pondering how
to keep them from being duplicated. I'm working on some ideas.

The four are:

inode_getsecurity
inode_setsecurity
task_prctl
xfrm_state_pol_flow_match

None of these are already just calling call_int_hook(), but I assume
they'll need further tweaks in the coming stacking.

To leave things as open-code-able as possible while still benefiting
from the macro consolidation, how about something like this:

lsm_hook_names.h:

LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity,
	 struct inode *inode, const char *name, void **buffer, bool alloc)

...

security.c:

#define LSM_RET_DEFAULT_void(DEFAULT, NAME)	/* */
#define LSM_RET_DEFAULT_int(DEFAULT, NAME)
	static const int NAME#_default = (DEFAULT);

#define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
	LSM_RET_DEFAULT_#RET(DEFAULT, NAME)
#include <linux/lsm_hook_names.h>
#undef LSM_HOOK
...

Then -EOPNOTSUPP is available as "inode_getsecurity_default":

int security_inode_getsecurity(struct inode *inode, const char *name,
			       void **buffer, bool alloc)
{
        struct security_hook_list *hp;
        int rc;

        if (unlikely(IS_PRIVATE(inode)))
                return inode_getsecurity_default;
        /*
         * Only one module will provide an attribute with a given name.
         */
        hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
                rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
                if (rc != inode_getsecurity_default)
                        return rc;
        }
        return inode_getsecurity_default;
}


On the other hand, it's only 4 non-default return codes, so maybe the
sync burden isn't very high?
Casey Schaufler March 24, 2020, 1:13 a.m. UTC | #6 
On 3/23/2020 9:44 AM, 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>
> Reviewed-by: Brendan Jackman <jackmanb@google.com>
> Reviewed-by: Florent Revest <revest@google.com>
> ---
>  security/Kconfig      | 10 ++++----
>  security/Makefile     |  2 ++
>  security/bpf/Makefile |  5 ++++
>  security/bpf/hooks.c  | 55 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 67 insertions(+), 5 deletions(-)
>  create mode 100644 security/bpf/Makefile
>  create mode 100644 security/bpf/hooks.c
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 2a1a2d396228..cd3cc7da3a55 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -277,11 +277,11 @@ endchoice
>  
>  config LSM
>  	string "Ordered list of enabled LSMs"
> -	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
> -	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
> -	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> -	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
> -	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> +	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> +	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> +	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> +	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
> +	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
>  	help
>  	  A comma-separated list of LSMs, in initialization order.
>  	  Any LSMs left off this list will be ignored. This can be
> diff --git a/security/Makefile b/security/Makefile
> index 746438499029..22e73a3482bd 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA)		+= yama
>  subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
>  subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
>  subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
> +subdir-$(CONFIG_BPF_LSM)		+= bpf
>  
>  # always enable default capabilities
>  obj-y					+= commoncap.o
> @@ -30,6 +31,7 @@ obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
>  obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
>  obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
>  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
> +obj-$(CONFIG_BPF_LSM)			+= bpf/
>  
>  # Object integrity file lists
>  subdir-$(CONFIG_INTEGRITY)		+= integrity
> diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> new file mode 100644
> index 000000000000..c7a89a962084
> --- /dev/null
> +++ b/security/bpf/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2020 Google LLC.
> +
> +obj-$(CONFIG_BPF_LSM) := hooks.o
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> new file mode 100644
> index 000000000000..68e5824868f9
> --- /dev/null
> +++ b/security/bpf/hooks.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * 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
> + */
> +noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const char *name,
> +				       void **buffer, bool alloc)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +noinline int bpf_lsm_inode_setsecurity(struct inode *inode, const char *name,
> +				       const void *value, size_t size,
> +				       int flags)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +noinline int bpf_lsm_task_prctl(int option, unsigned long arg2,
> +				unsigned long arg3, unsigned long arg4,
> +				unsigned long arg5)
> +{
> +	return -ENOSYS;
> +}
> +
> +noinline int bpf_lsm_xfrm_state_pol_flow_match(struct xfrm_state *x,
> +					       struct xfrm_policy *xp,
> +					       const struct flowi *fl)
> +{
> +	return 1;
> +}
> +
> +static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
> +	#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
> +	#include <linux/lsm_hook_names.h>
> +	#undef LSM_HOOK
> +};
> +
> +static int __init bpf_lsm_init(void)
> +{
> +	security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
> +	pr_info("LSM support for eBPF active\n");
> +	return 0;
> +}
> +
> +DEFINE_LSM(bpf) = {
> +	.name = "bpf",
> +	.init = bpf_lsm_init,

Have you given up on the "BPF must be last" requirement?

> +};
KP Singh March 24, 2020, 1:52 a.m. UTC | #7 
On 23-Mär 18:13, Casey Schaufler wrote:
> On 3/23/2020 9:44 AM, 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
> >
> > +	return 0;

[...]

> > +}
> > +
> > +DEFINE_LSM(bpf) = {
> > +	.name = "bpf",
> > +	.init = bpf_lsm_init,
> 
> Have you given up on the "BPF must be last" requirement?

Yes, we dropped it for as the BPF programs require CAP_SYS_ADMIN
anwyays so the position ~shouldn't~ matter. (based on some of the
discussions we had on the BPF_MODIFY_RETURN patches).

However, This can be added later (in a separate patch) if really
deemed necessary.

- KP

> 
> > +};
Stephen Smalley March 24, 2020, 2:37 p.m. UTC | #8 
On Mon, Mar 23, 2020 at 9:52 PM KP Singh <kpsingh@chromium.org> wrote:
>
> On 23-Mär 18:13, Casey Schaufler wrote:
> > On 3/23/2020 9:44 AM, 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
> > >
> > > +   return 0;
>
> [...]
>
> > > +}
> > > +
> > > +DEFINE_LSM(bpf) = {
> > > +   .name = "bpf",
> > > +   .init = bpf_lsm_init,
> >
> > Have you given up on the "BPF must be last" requirement?
>
> Yes, we dropped it for as the BPF programs require CAP_SYS_ADMIN
> anwyays so the position ~shouldn't~ matter. (based on some of the
> discussions we had on the BPF_MODIFY_RETURN patches).
>
> However, This can be added later (in a separate patch) if really
> deemed necessary.

It matters for SELinux, as I previously explained.  A process that has
CAP_SYS_ADMIN is not assumed to be able to circumvent MAC policy.
And executing prior to SELinux allows the bpf program to access and
potentially leak to userspace information that wouldn't be visible to
the
process itself. However, I thought you were handling the order issue
by putting it last in the list of lsms?
KP Singh March 24, 2020, 2:42 p.m. UTC | #9 
On 24-Mär 10:37, Stephen Smalley wrote:
> On Mon, Mar 23, 2020 at 9:52 PM KP Singh <kpsingh@chromium.org> wrote:
> >
> > On 23-Mär 18:13, Casey Schaufler wrote:
> > > On 3/23/2020 9:44 AM, 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
> > > >
> > > > +   return 0;
> >
> > [...]
> >
> > > > +}
> > > > +
> > > > +DEFINE_LSM(bpf) = {
> > > > +   .name = "bpf",
> > > > +   .init = bpf_lsm_init,
> > >
> > > Have you given up on the "BPF must be last" requirement?
> >
> > Yes, we dropped it for as the BPF programs require CAP_SYS_ADMIN
> > anwyays so the position ~shouldn't~ matter. (based on some of the
> > discussions we had on the BPF_MODIFY_RETURN patches).
> >
> > However, This can be added later (in a separate patch) if really
> > deemed necessary.
> 
> It matters for SELinux, as I previously explained.  A process that has
> CAP_SYS_ADMIN is not assumed to be able to circumvent MAC policy.
> And executing prior to SELinux allows the bpf program to access and
> potentially leak to userspace information that wouldn't be visible to
> the
> process itself. However, I thought you were handling the order issue
> by putting it last in the list of lsms?

We can still do that if it does not work for SELinux.

Would it be okay to add bpf as LSM_ORDER_LAST?

LSMs like Landlock can then add LSM_ORDER_UNPRIVILEGED to even end up
after bpf?

- KP
Stephen Smalley March 24, 2020, 2:51 p.m. UTC | #10 
On Tue, Mar 24, 2020 at 10:42 AM KP Singh <kpsingh@chromium.org> wrote:
>
> On 24-Mär 10:37, Stephen Smalley wrote:
> > On Mon, Mar 23, 2020 at 9:52 PM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > On 23-Mär 18:13, Casey Schaufler wrote:
> > > > Have you given up on the "BPF must be last" requirement?
> > >
> > > Yes, we dropped it for as the BPF programs require CAP_SYS_ADMIN
> > > anwyays so the position ~shouldn't~ matter. (based on some of the
> > > discussions we had on the BPF_MODIFY_RETURN patches).
> > >
> > > However, This can be added later (in a separate patch) if really
> > > deemed necessary.
> >
> > It matters for SELinux, as I previously explained.  A process that has
> > CAP_SYS_ADMIN is not assumed to be able to circumvent MAC policy.
> > And executing prior to SELinux allows the bpf program to access and
> > potentially leak to userspace information that wouldn't be visible to
> > the
> > process itself. However, I thought you were handling the order issue
> > by putting it last in the list of lsms?
>
> We can still do that if it does not work for SELinux.
>
> Would it be okay to add bpf as LSM_ORDER_LAST?
>
> LSMs like Landlock can then add LSM_ORDER_UNPRIVILEGED to even end up
> after bpf?

I guess the question is whether we need an explicit LSM_ORDER_LAST or
can just handle it via the default
values for the lsm= parameter, where you are already placing bpf last
IIUC?  If someone can mess with the kernel boot
parameters, they already have options to mess with SELinux, so it is no worse...
KP Singh March 24, 2020, 2:51 p.m. UTC | #11 
On 24-Mär 10:51, Stephen Smalley wrote:
> On Tue, Mar 24, 2020 at 10:42 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > On 24-Mär 10:37, Stephen Smalley wrote:
> > > On Mon, Mar 23, 2020 at 9:52 PM KP Singh <kpsingh@chromium.org> wrote:
> > > >
> > > > On 23-Mär 18:13, Casey Schaufler wrote:
> > > > > Have you given up on the "BPF must be last" requirement?
> > > >
> > > > Yes, we dropped it for as the BPF programs require CAP_SYS_ADMIN
> > > > anwyays so the position ~shouldn't~ matter. (based on some of the
> > > > discussions we had on the BPF_MODIFY_RETURN patches).
> > > >
> > > > However, This can be added later (in a separate patch) if really
> > > > deemed necessary.
> > >
> > > It matters for SELinux, as I previously explained.  A process that has
> > > CAP_SYS_ADMIN is not assumed to be able to circumvent MAC policy.
> > > And executing prior to SELinux allows the bpf program to access and
> > > potentially leak to userspace information that wouldn't be visible to
> > > the
> > > process itself. However, I thought you were handling the order issue
> > > by putting it last in the list of lsms?
> >
> > We can still do that if it does not work for SELinux.
> >
> > Would it be okay to add bpf as LSM_ORDER_LAST?
> >
> > LSMs like Landlock can then add LSM_ORDER_UNPRIVILEGED to even end up
> > after bpf?
> 
> I guess the question is whether we need an explicit LSM_ORDER_LAST or
> can just handle it via the default
> values for the lsm= parameter, where you are already placing bpf last
> IIUC?  If someone can mess with the kernel boot
> parameters, they already have options to mess with SELinux, so it is no worse...

Yeah, we do add BPF as the last LSM in the default list. So, I will
avoid adding LSM_ORDER_LAST for now.

- KP
Kees Cook March 24, 2020, 5:57 p.m. UTC | #12 
On Tue, Mar 24, 2020 at 03:51:55PM +0100, KP Singh wrote:
> On 24-Mär 10:51, Stephen Smalley wrote:
> > On Tue, Mar 24, 2020 at 10:42 AM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > On 24-Mär 10:37, Stephen Smalley wrote:
> > > > On Mon, Mar 23, 2020 at 9:52 PM KP Singh <kpsingh@chromium.org> wrote:
> > > > >
> > > > > On 23-Mär 18:13, Casey Schaufler wrote:
> > > > > > Have you given up on the "BPF must be last" requirement?
> > > > >
> > > > > Yes, we dropped it for as the BPF programs require CAP_SYS_ADMIN
> > > > > anwyays so the position ~shouldn't~ matter. (based on some of the
> > > > > discussions we had on the BPF_MODIFY_RETURN patches).
> > > > >
> > > > > However, This can be added later (in a separate patch) if really
> > > > > deemed necessary.
> > > >
> > > > It matters for SELinux, as I previously explained.  A process that has
> > > > CAP_SYS_ADMIN is not assumed to be able to circumvent MAC policy.
> > > > And executing prior to SELinux allows the bpf program to access and
> > > > potentially leak to userspace information that wouldn't be visible to
> > > > the
> > > > process itself. However, I thought you were handling the order issue
> > > > by putting it last in the list of lsms?
> > >
> > > We can still do that if it does not work for SELinux.
> > >
> > > Would it be okay to add bpf as LSM_ORDER_LAST?
> > >
> > > LSMs like Landlock can then add LSM_ORDER_UNPRIVILEGED to even end up
> > > after bpf?
> > 
> > I guess the question is whether we need an explicit LSM_ORDER_LAST or
> > can just handle it via the default
> > values for the lsm= parameter, where you are already placing bpf last
> > IIUC?  If someone can mess with the kernel boot
> > parameters, they already have options to mess with SELinux, so it is no worse...
> 
> Yeah, we do add BPF as the last LSM in the default list. So, I will
> avoid adding LSM_ORDER_LAST for now.

FWIW, this is my preference as well. If there ends up being a stronger
need, then we can implement LSM_ORDER_LAST at that time.
diff mbox series

Patch

diff --git a/security/Kconfig b/security/Kconfig
index 2a1a2d396228..cd3cc7da3a55 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -277,11 +277,11 @@  endchoice
 
 config LSM
 	string "Ordered list of enabled LSMs"
-	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
-	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
-	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
-	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
-	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
+	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
+	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
+	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
+	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
+	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
 	help
 	  A comma-separated list of LSMs, in initialization order.
 	  Any LSMs left off this list will be ignored. This can be
diff --git a/security/Makefile b/security/Makefile
index 746438499029..22e73a3482bd 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -12,6 +12,7 @@  subdir-$(CONFIG_SECURITY_YAMA)		+= yama
 subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
 subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
 subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
+subdir-$(CONFIG_BPF_LSM)		+= bpf
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -30,6 +31,7 @@  obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
 obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
 obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
+obj-$(CONFIG_BPF_LSM)			+= bpf/
 
 # Object integrity file lists
 subdir-$(CONFIG_INTEGRITY)		+= integrity
diff --git a/security/bpf/Makefile b/security/bpf/Makefile
new file mode 100644
index 000000000000..c7a89a962084
--- /dev/null
+++ b/security/bpf/Makefile
@@ -0,0 +1,5 @@ 
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2020 Google LLC.
+
+obj-$(CONFIG_BPF_LSM) := hooks.o
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
new file mode 100644
index 000000000000..68e5824868f9
--- /dev/null
+++ b/security/bpf/hooks.c
@@ -0,0 +1,55 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * 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
+ */
+noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const char *name,
+				       void **buffer, bool alloc)
+{
+	return -EOPNOTSUPP;
+}
+
+noinline int bpf_lsm_inode_setsecurity(struct inode *inode, const char *name,
+				       const void *value, size_t size,
+				       int flags)
+{
+	return -EOPNOTSUPP;
+}
+
+noinline int bpf_lsm_task_prctl(int option, unsigned long arg2,
+				unsigned long arg3, unsigned long arg4,
+				unsigned long arg5)
+{
+	return -ENOSYS;
+}
+
+noinline int bpf_lsm_xfrm_state_pol_flow_match(struct xfrm_state *x,
+					       struct xfrm_policy *xp,
+					       const struct flowi *fl)
+{
+	return 1;
+}
+
+static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
+	#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
+	#include <linux/lsm_hook_names.h>
+	#undef LSM_HOOK
+};
+
+static int __init bpf_lsm_init(void)
+{
+	security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
+	pr_info("LSM support for eBPF active\n");
+	return 0;
+}
+
+DEFINE_LSM(bpf) = {
+	.name = "bpf",
+	.init = bpf_lsm_init,
+};