| Message ID | 20200323164415.12943-6-kpsingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MAC and Audit policy using eBPF (KRSI) | expand |
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 >
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
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
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 >>
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?
On 3/23/2020 2:44 PM, Kees Cook wrote: > 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? That's not too terrible, I suppose. What would you be thinking for the calls that do use call_int_hook()? rc = call_int_hook(something, something_default, goodnesses); or embedded in the macro: rc = call_int_hook(something, goodnesses);
On Mon, Mar 23, 2020 at 02:58:18PM -0700, Casey Schaufler wrote: > That's not too terrible, I suppose. What would you be thinking for > the calls that do use call_int_hook()? > > rc = call_int_hook(something, something_default, goodnesses); > > or embedded in the macro: > > rc = call_int_hook(something, goodnesses); Oh yes, good point. The hook call already knows the name, so: #define call_int_hook(FUNC, ...) ({ \ int RC = FUNC#_default; \ ...
On 3/23/2020 3:12 PM, Kees Cook wrote: > On Mon, Mar 23, 2020 at 02:58:18PM -0700, Casey Schaufler wrote: >> That's not too terrible, I suppose. What would you be thinking for >> the calls that do use call_int_hook()? >> >> rc = call_int_hook(something, something_default, goodnesses); >> >> or embedded in the macro: >> >> rc = call_int_hook(something, goodnesses); > Oh yes, good point. The hook call already knows the name, so: > > #define call_int_hook(FUNC, ...) ({ \ > int RC = FUNC#_default; \ > ... That makes the most sense, I think. It's getting a little heavy on hidden magic, but we do tend to have a pretty good set of eyes watching when a new hook is proposed. I would expect the changes to call_int_hook() and its callers should be made when default is added to LSM_HOOK, not after.
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? > +};
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 > > > +};
On 23-Mär 15:12, Kees Cook wrote: > On Mon, Mar 23, 2020 at 02:58:18PM -0700, Casey Schaufler wrote: > > That's not too terrible, I suppose. What would you be thinking for > > the calls that do use call_int_hook()? > > > > rc = call_int_hook(something, something_default, goodnesses); > > > > or embedded in the macro: > > > > rc = call_int_hook(something, goodnesses); > > Oh yes, good point. The hook call already knows the name, so: > > #define call_int_hook(FUNC, ...) ({ \ > int RC = FUNC#_default; \ Excellent ideas, I will spin up the next revision with these changes. - KP > ... > > > -- > Kees Cook > >
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?
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
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...
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
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.
On 23-Mär 15:12, Kees Cook wrote: > On Mon, Mar 23, 2020 at 02:58:18PM -0700, Casey Schaufler wrote: > > That's not too terrible, I suppose. What would you be thinking for > > the calls that do use call_int_hook()? > > > > rc = call_int_hook(something, something_default, goodnesses); > > > > or embedded in the macro: > > > > rc = call_int_hook(something, goodnesses); > > Oh yes, good point. The hook call already knows the name, so: I learnt this the hard way that IRC that is passed to the call_int_hook macro is not the same as the default value for a hook call_int_hook accomdates for a different return value when no hook is implemented, but it does expect the default value of the hook to be 0 as it compares the return value of the callbacks to 0 instead of the default value whereas these special cases compare it with the default value. For example: If we define the default_value of the secid_to_secctx to -EOPNOTSUPP, it changes the behaviour and the BPF hook, which returns this default value always results in a failure. I noticed this when I saw a bunch of messages on my VM: audit: error in audit_log_task_context which comes from audit_log_task_context and calls security_secid_to_secctx which ends up being always denied by BPF. In anycase, I am still adding the default value in LSM_HOOK and using them in the following hooks: getprocattr -EINVAL inode_getsecurity -EOPNOTSUPP inode_setsecurity -EOPNOTSUPP setprocattr -EINVAL task_prctl -ENOSYS xfrm_state_pol_flow_match 1 Will send v6 out with these changes. - KP > > #define call_int_hook(FUNC, ...) ({ \ > int RC = FUNC#_default; \ > ... > > > -- > Kees Cook > >
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, +};