diff mbox series

[bpf-next,v4,3/8] bpf: lsm: provide attachment points for BPF LSM programs

Message ID 20200220175250.10795-4-kpsingh@chromium.org (mailing list archive)
State New, archived
Headers show
Series MAC and Audit policy using eBPF (KRSI) | expand

Commit Message

KP Singh Feb. 20, 2020, 5:52 p.m. UTC
From: KP Singh <kpsingh@google.com>

The BPF LSM programs are implemented as fexit trampolines to avoid the
overhead of retpolines. These programs cannot be attached to security_*
wrappers as there are quite a few security_* functions that do more than
just calling the LSM callbacks.

This was discussed on the lists in:

  https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266

Adding a NOP callback after all the static LSM callbacks are called has
the following benefits:

- The BPF programs run at the right stage of the security_* wrappers.
- They run after all the static LSM hooks allowed the operation,
  therefore cannot allow an action that was already denied.

There are some hooks which do not call call_int_hooks or
call_void_hooks. It's not possible to call the bpf_lsm_* functions
without checking if there is BPF LSM program attached to these hooks.
This is added further in a subsequent patch. For now, these hooks are
marked as NO_BPF (i.e. attachment of BPF programs is not possible).

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/bpf_lsm.h | 34 ++++++++++++++++++++++++++++++++++
 kernel/bpf/bpf_lsm.c    | 16 ++++++++++++++++
 security/security.c     |  3 +++
 3 files changed, 53 insertions(+)
 create mode 100644 include/linux/bpf_lsm.h

Comments

Casey Schaufler Feb. 20, 2020, 11:49 p.m. UTC | #1
On 2/20/2020 9:52 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>

Sorry about the heavy list pruning - the original set
blows thunderbird up.

>
> The BPF LSM programs are implemented as fexit trampolines to avoid the
> overhead of retpolines. These programs cannot be attached to security_*
> wrappers as there are quite a few security_* functions that do more than
> just calling the LSM callbacks.
>
> This was discussed on the lists in:
>
>   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
>
> Adding a NOP callback after all the static LSM callbacks are called has
> the following benefits:
>
> - The BPF programs run at the right stage of the security_* wrappers.
> - They run after all the static LSM hooks allowed the operation,
>   therefore cannot allow an action that was already denied.

I still say that the special call-out to BPF is unnecessary.
I remain unconvinced by the arguments. You aren't doing anything
so special that the general mechanism won't work.

>
> There are some hooks which do not call call_int_hooks or
> call_void_hooks. It's not possible to call the bpf_lsm_* functions
> without checking if there is BPF LSM program attached to these hooks.
> This is added further in a subsequent patch. For now, these hooks are
> marked as NO_BPF (i.e. attachment of BPF programs is not possible).
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  include/linux/bpf_lsm.h | 34 ++++++++++++++++++++++++++++++++++
>  kernel/bpf/bpf_lsm.c    | 16 ++++++++++++++++
>  security/security.c     |  3 +++
>  3 files changed, 53 insertions(+)
>  create mode 100644 include/linux/bpf_lsm.h
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> new file mode 100644
> index 000000000000..f867f72f6aa9
> --- /dev/null
> +++ b/include/linux/bpf_lsm.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright 2019 Google LLC.
> + */
> +
> +#ifndef _LINUX_BPF_LSM_H
> +#define _LINUX_BPF_LSM_H
> +
> +#include <linux/bpf.h>
> +
> +#ifdef CONFIG_BPF_LSM
> +
> +#define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__);
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
> +
> +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) bpf_lsm_##FUNC(__VA_ARGS__)
> +#define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) ({				\
> +	do {								\
> +		if (RC == 0)						\
> +			RC = bpf_lsm_##FUNC(__VA_ARGS__);		\
> +	} while (0);							\
> +	RC;								\
> +})
> +
> +#else /* !CONFIG_BPF_LSM */
> +
> +#define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) (RC)
> +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...)
> +
> +#endif /* CONFIG_BPF_LSM */
> +
> +#endif /* _LINUX_BPF_LSM_H */
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index affb6941622e..abc847c9b9a1 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -7,6 +7,22 @@
>  #include <linux/filter.h>
>  #include <linux/bpf.h>
>  #include <linux/btf.h>
> +#include <linux/bpf_lsm.h>
> +
> +/* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> + * function where a BPF program can be attached as an fexit trampoline.
> + */
> +#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
> +#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__)  \
> +{									  \
> +	return 0;							  \
> +}
> +
> +#define LSM_HOOK_void(NAME, ...) \
> +	noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
> +
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
>  
>  const struct bpf_prog_ops lsm_prog_ops = {
>  };
> diff --git a/security/security.c b/security/security.c
> index 565bc9b67276..aa111392a700 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,7 @@
>  #include <linux/string.h>
>  #include <linux/msg.h>
>  #include <net/flow.h>
> +#include <linux/bpf_lsm.h>
>  
>  #define MAX_LSM_EVM_XATTR	2
>  
> @@ -684,6 +685,7 @@ 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__);		\
> +		RUN_BPF_LSM_VOID_PROGS(FUNC, __VA_ARGS__);	\

>  	} while (0)
>  
>  #define call_int_hook(FUNC, IRC, ...) ({			\
> @@ -696,6 +698,7 @@ static void __init lsm_early_task(struct task_struct *task)
>  			if (RC != 0)				\
>  				break;				\
>  		}						\
> +		RC = RUN_BPF_LSM_INT_PROGS(RC, FUNC, __VA_ARGS__); \
>  	} while (0);						\
>  	RC;							\
>  })
Alexei Starovoitov Feb. 21, 2020, 2:25 a.m. UTC | #2
On Thu, Feb 20, 2020 at 06:52:45PM +0100, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> The BPF LSM programs are implemented as fexit trampolines to avoid the
> overhead of retpolines. These programs cannot be attached to security_*
> wrappers as there are quite a few security_* functions that do more than
> just calling the LSM callbacks.
> 
> This was discussed on the lists in:
> 
>   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> 
> Adding a NOP callback after all the static LSM callbacks are called has
> the following benefits:
> 
> - The BPF programs run at the right stage of the security_* wrappers.
> - They run after all the static LSM hooks allowed the operation,
>   therefore cannot allow an action that was already denied.
> 
> There are some hooks which do not call call_int_hooks or
> call_void_hooks. It's not possible to call the bpf_lsm_* functions
> without checking if there is BPF LSM program attached to these hooks.
> This is added further in a subsequent patch. For now, these hooks are
> marked as NO_BPF (i.e. attachment of BPF programs is not possible).

the commit log doesn't match the code.

> +
> +/* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> + * function where a BPF program can be attached as an fexit trampoline.
> + */
> +#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
> +#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__)  \

Did you check generated asm?
I think I saw cases when gcc ignored 'noinline' when function is defined in the
same file and still performed inlining while keeping the function body.
To be safe I think __weak is necessary. That will guarantee noinline.

And please reduce your cc next time. It's way too long.
KP Singh Feb. 21, 2020, 11:44 a.m. UTC | #3
On 20-Feb 15:49, Casey Schaufler wrote:
> On 2/20/2020 9:52 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> 
> Sorry about the heavy list pruning - the original set
> blows thunderbird up.
> 
> >
> > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > overhead of retpolines. These programs cannot be attached to security_*
> > wrappers as there are quite a few security_* functions that do more than
> > just calling the LSM callbacks.
> >
> > This was discussed on the lists in:
> >
> >   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> >
> > Adding a NOP callback after all the static LSM callbacks are called has
> > the following benefits:
> >
> > - The BPF programs run at the right stage of the security_* wrappers.
> > - They run after all the static LSM hooks allowed the operation,
> >   therefore cannot allow an action that was already denied.
> 
> I still say that the special call-out to BPF is unnecessary.
> I remain unconvinced by the arguments. You aren't doing anything
> so special that the general mechanism won't work.

The existing mechanism would work functionally, but the cost of an
indirect call for all the hooks, even those that are completely unused
is not really acceptable for KRSI’s use cases. It’s easy to avoid and
I do think that what we’re doing here (with hooks being defined at
runtime) has significant functional differences from existing LSMs.

- KP

> 
> >
> > There are some hooks which do not call call_int_hooks or
> > call_void_hooks. It's not possible to call the bpf_lsm_* functions
> > without checking if there is BPF LSM program attached to these hooks.
> > This is added further in a subsequent patch. For now, these hooks are
> > marked as NO_BPF (i.e. attachment of BPF programs is not possible).
> >

[...]
KP Singh Feb. 21, 2020, 11:47 a.m. UTC | #4
On 20-Feb 18:25, Alexei Starovoitov wrote:
> On Thu, Feb 20, 2020 at 06:52:45PM +0100, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> > 
> > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > overhead of retpolines. These programs cannot be attached to security_*
> > wrappers as there are quite a few security_* functions that do more than
> > just calling the LSM callbacks.
> > 
> > This was discussed on the lists in:
> > 
> >   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> > 
> > Adding a NOP callback after all the static LSM callbacks are called has
> > the following benefits:
> > 
> > - The BPF programs run at the right stage of the security_* wrappers.
> > - They run after all the static LSM hooks allowed the operation,
> >   therefore cannot allow an action that was already denied.
> > 
> > There are some hooks which do not call call_int_hooks or
> > call_void_hooks. It's not possible to call the bpf_lsm_* functions
> > without checking if there is BPF LSM program attached to these hooks.
> > This is added further in a subsequent patch. For now, these hooks are
> > marked as NO_BPF (i.e. attachment of BPF programs is not possible).
> 
> the commit log doesn't match the code.

Fixed. Thanks!

> 
> > +
> > +/* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > + * function where a BPF program can be attached as an fexit trampoline.
> > + */
> > +#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
> > +#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__)  \
> 
> Did you check generated asm?
> I think I saw cases when gcc ignored 'noinline' when function is defined in the
> same file and still performed inlining while keeping the function body.
> To be safe I think __weak is necessary. That will guarantee noinline.

Sure, will change it to __weak.

> 
> And please reduce your cc next time. It's way too long.

Will do.

- KP
Casey Schaufler Feb. 21, 2020, 6:23 p.m. UTC | #5
On 2/21/2020 3:44 AM, KP Singh wrote:
> On 20-Feb 15:49, Casey Schaufler wrote:
>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>> Sorry about the heavy list pruning - the original set
>> blows thunderbird up.
>>
>>> The BPF LSM programs are implemented as fexit trampolines to avoid the
>>> overhead of retpolines. These programs cannot be attached to security_*
>>> wrappers as there are quite a few security_* functions that do more than
>>> just calling the LSM callbacks.
>>>
>>> This was discussed on the lists in:
>>>
>>>   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
>>>
>>> Adding a NOP callback after all the static LSM callbacks are called has
>>> the following benefits:
>>>
>>> - The BPF programs run at the right stage of the security_* wrappers.
>>> - They run after all the static LSM hooks allowed the operation,
>>>   therefore cannot allow an action that was already denied.
>> I still say that the special call-out to BPF is unnecessary.
>> I remain unconvinced by the arguments. You aren't doing anything
>> so special that the general mechanism won't work.
> The existing mechanism would work functionally, but the cost of an
> indirect call for all the hooks, even those that are completely unused
> is not really acceptable for KRSI’s use cases.

Are you at all familiar with the way LSMs where installed
before the current list infrastructure? Every interface had
a hook that got called, even if the installed module did not
provide one. That was deemed acceptable for a good long time.

Way back in the early days of the stacking effort I seriously
considered implementing a new security module that would do
the stacking, and leave the infrastructure alone. Very much
like what you're proposing for BPF modules. It would have worked,
but the list model works better.

>  It’s easy to avoid and
> I do think that what we’re doing here (with hooks being defined at
> runtime) has significant functional differences from existing LSMs.

KRSI isn't all that different from the other modules.
The way you specify where system policy is restricted
and under which circumstances is different. You're trying
to be extremely general, beyond the Mandatory Access Control
claims of the existing modules. But really, there's nothing
all that special.

I know that you don't want to be making a lot of checks on
empty BPF program lists. You've come up with clever hacks
to avoid doing so. But the cleverer the hack, the more likely
it is to haunt someone else later. It probably won't cause
KRSI any grief, but you can bet someone will take it in the
chin.
Kees Cook Feb. 22, 2020, 4:22 a.m. UTC | #6
On Thu, Feb 20, 2020 at 03:49:05PM -0800, Casey Schaufler wrote:
> On 2/20/2020 9:52 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> 
> Sorry about the heavy list pruning - the original set
> blows thunderbird up.

(I've added some people back; I had to dig this thread back out of lkml
since I didn't get a direct copy...)

> > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > overhead of retpolines. These programs cannot be attached to security_*
> > wrappers as there are quite a few security_* functions that do more than
> > just calling the LSM callbacks.
> >
> > This was discussed on the lists in:
> >
> >   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> >
> > Adding a NOP callback after all the static LSM callbacks are called has
> > the following benefits:
> >
> > - The BPF programs run at the right stage of the security_* wrappers.
> > - They run after all the static LSM hooks allowed the operation,
> >   therefore cannot allow an action that was already denied.
> 
> I still say that the special call-out to BPF is unnecessary.
> I remain unconvinced by the arguments. You aren't doing anything
> so special that the general mechanism won't work.

If I'm understanding this correctly, there are two issues:

1- BPF needs to be run last due to fexit trampolines (?)

2- BPF hooks don't know what may be attached at any given time, so
   ALL LSM hooks need to be universally hooked. THIS turns out to create
   a measurable performance problem in that the cost of the indirect call
   on the (mostly/usually) empty BPF policy is too high.

"1" can be solved a lot of ways, and doesn't seem to be a debated part
of this series.

"2" is interesting -- it creates a performance problem for EVERYONE that
builds in this kernel feature, regardless of them using it. Excepting
SELinux, "traditional" LSMs tends to be relatively sparse in their hooking:

$ grep '^      struct hlist_head' include/linux/lsm_hooks.h | wc -l
230
$ for i in apparmor loadpin lockdown safesetid selinux smack tomoyo yama ; \
  do echo -n "$i " && (cd $i && git grep LSM_HOOK_INIT | wc -l) ; done
apparmor   68
loadpin     3
lockdown    1
safesetid   2
selinux   202
smack     108
tomoyo     28
yama        4

So, trying to avoid the indirect calls is, as you say, an optimization,
but it might be a needed one due to the other limitations.

To me, some questions present themselves:

a) What, exactly, are the performance characteristics of:
	"before"
	"with indirect calls"
	"with static keys optimization"

b) Would there actually be a global benefit to using the static keys
   optimization for other LSMs? (Especially given that they're already
   sparsely populated and policy likely determines utility -- all the
   LSMs would just turn ON all their static keys or turn off ALL their
   static keys depending on having policy loaded.)

If static keys are justified for KRSI (by "a") then it seems the approach
here should stand. If "b" is also true, then we need an additional
series to apply this optimization for the other LSMs (but that seems
distinctly separate from THIS series).
Alexei Starovoitov Feb. 23, 2020, 10:08 p.m. UTC | #7
On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
> 
> If I'm understanding this correctly, there are two issues:
> 
> 1- BPF needs to be run last due to fexit trampolines (?)

no.
The placement of nop call can be anywhere.
BPF trampoline is automagically converting nop call into a sequence
of directly invoked BPF programs.
No link list traversals and no indirect calls in run-time.

> 2- BPF hooks don't know what may be attached at any given time, so
>    ALL LSM hooks need to be universally hooked. THIS turns out to create
>    a measurable performance problem in that the cost of the indirect call
>    on the (mostly/usually) empty BPF policy is too high.

also no.

> So, trying to avoid the indirect calls is, as you say, an optimization,
> but it might be a needed one due to the other limitations.

I'm convinced that avoiding the cost of retpoline in critical path is a
requirement for any new infrastructure in the kernel.
Not only for security, but for any new infra.
Networking stack converted all such places to conditional calls.
In BPF land we converted indirect calls to direct jumps and direct calls.
It took two years to do so. Adding new indirect calls is not an option.
I'm eagerly waiting for Peter's static_call patches to land to convert
a lot more indirect calls. May be existing LSMs will take advantage
of static_call patches too, but static_call is not an option for BPF.
That's why we introduced BPF trampoline in the last kernel release.

> b) Would there actually be a global benefit to using the static keys
>    optimization for other LSMs?

Yes. Just compiling with CONFIG_SECURITY adds "if (hlist_empty)" check
for every hook. Some of those hooks are in critical path. This load+cmp
can be avoided with static_key optimization. I think it's worth doing.

> If static keys are justified for KRSI

I really like that KRSI costs absolutely zero when it's not enabled.
Attaching BPF prog to one hook preserves zero cost for all other hooks.
And when one hook is BPF powered it's using direct call instead of
super expensive retpoline.

Overall this patch set looks good to me. There was a minor issue with prog
accounting. I expect only that bit to be fixed in v5.
Casey Schaufler Feb. 24, 2020, 4:09 p.m. UTC | #8
On 2/21/2020 8:22 PM, Kees Cook wrote:
> On Thu, Feb 20, 2020 at 03:49:05PM -0800, Casey Schaufler wrote:
>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>> Sorry about the heavy list pruning - the original set
>> blows thunderbird up.
> (I've added some people back; I had to dig this thread back out of lkml
> since I didn't get a direct copy...)
>
>>> The BPF LSM programs are implemented as fexit trampolines to avoid the
>>> overhead of retpolines. These programs cannot be attached to security_*
>>> wrappers as there are quite a few security_* functions that do more than
>>> just calling the LSM callbacks.
>>>
>>> This was discussed on the lists in:
>>>
>>>   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
>>>
>>> Adding a NOP callback after all the static LSM callbacks are called has
>>> the following benefits:
>>>
>>> - The BPF programs run at the right stage of the security_* wrappers.
>>> - They run after all the static LSM hooks allowed the operation,
>>>   therefore cannot allow an action that was already denied.
>> I still say that the special call-out to BPF is unnecessary.
>> I remain unconvinced by the arguments. You aren't doing anything
>> so special that the general mechanism won't work.
> If I'm understanding this correctly, there are two issues:
>
> 1- BPF needs to be run last due to fexit trampolines (?)

That's my understanding. As you mention below, there are many
ways to skin that cat.

> 2- BPF hooks don't know what may be attached at any given time, so
>    ALL LSM hooks need to be universally hooked.

Right. But that's exactly what we had before we switched to
the hook lists for stacking. It was perfectly acceptable, and
was accepted that way, for years. People even objected to it
being changed.

>  THIS turns out to create
>    a measurable performance problem in that the cost of the indirect call
>    on the (mostly/usually) empty BPF policy is too high.

Right. Except that it was deemed acceptable back before stacking.
What has changed? 

>
> "1" can be solved a lot of ways, and doesn't seem to be a debated part
> of this series.
>
> "2" is interesting -- it creates a performance problem for EVERYONE that
> builds in this kernel feature, regardless of them using it. Excepting
> SELinux, "traditional" LSMs tends to be relatively sparse in their hooking:
>
> $ grep '^      struct hlist_head' include/linux/lsm_hooks.h | wc -l
> 230
> $ for i in apparmor loadpin lockdown safesetid selinux smack tomoyo yama ; \
>   do echo -n "$i " && (cd $i && git grep LSM_HOOK_INIT | wc -l) ; done
> apparmor   68
> loadpin     3
> lockdown    1
> safesetid   2
> selinux   202
> smack     108
> tomoyo     28
> yama        4
>
> So, trying to avoid the indirect calls is, as you say, an optimization,
> but it might be a needed one due to the other limitations.
>
> To me, some questions present themselves:
>
> a) What, exactly, are the performance characteristics of:
> 	"before"
> 	"with indirect calls"
> 	"with static keys optimization"
>
> b) Would there actually be a global benefit to using the static keys
>    optimization for other LSMs? (Especially given that they're already
>    sparsely populated and policy likely determines utility -- all the
>    LSMs would just turn ON all their static keys or turn off ALL their
>    static keys depending on having policy loaded.)
>
> If static keys are justified for KRSI (by "a") then it seems the approach
> here should stand. If "b" is also true, then we need an additional
> series to apply this optimization for the other LSMs (but that seems
> distinctly separate from THIS series).
>
Casey Schaufler Feb. 24, 2020, 4:32 p.m. UTC | #9
On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
>> If I'm understanding this correctly, there are two issues:
>>
>> 1- BPF needs to be run last due to fexit trampolines (?)
> no.
> The placement of nop call can be anywhere.
> BPF trampoline is automagically converting nop call into a sequence
> of directly invoked BPF programs.
> No link list traversals and no indirect calls in run-time.

Then why the insistence that it be last?

>> 2- BPF hooks don't know what may be attached at any given time, so
>>    ALL LSM hooks need to be universally hooked. THIS turns out to create
>>    a measurable performance problem in that the cost of the indirect call
>>    on the (mostly/usually) empty BPF policy is too high.
> also no.

Um, then why not use the infrastructure as is?

>> So, trying to avoid the indirect calls is, as you say, an optimization,
>> but it might be a needed one due to the other limitations.
> I'm convinced that avoiding the cost of retpoline in critical path is a
> requirement for any new infrastructure in the kernel.

Sorry, I haven't gotten that memo.

> Not only for security, but for any new infra.

The LSM infrastructure ain't new.

> Networking stack converted all such places to conditional calls.
> In BPF land we converted indirect calls to direct jumps and direct calls.
> It took two years to do so. Adding new indirect calls is not an option.
> I'm eagerly waiting for Peter's static_call patches to land to convert
> a lot more indirect calls. May be existing LSMs will take advantage
> of static_call patches too, but static_call is not an option for BPF.
> That's why we introduced BPF trampoline in the last kernel release.

Sorry, but I don't see how BPF is so overwhelmingly special.

>> b) Would there actually be a global benefit to using the static keys
>>    optimization for other LSMs?
> Yes. Just compiling with CONFIG_SECURITY adds "if (hlist_empty)" check
> for every hook.

Err, no, it doesn't. It does an hlish_for_each_entry(), which
may be the equivalent on an empty list, but let's not go around
spreading misinformation.

>  Some of those hooks are in critical path. This load+cmp
> can be avoided with static_key optimization. I think it's worth doing.

I admit to being unfamiliar with the static_key implementation,
but if it would work for a list of hooks rather than a singe hook,
I'm all ears.

>> If static keys are justified for KRSI
> I really like that KRSI costs absolutely zero when it's not enabled.

And I dislike that there's security module specific code in security.c,
security.h and/or lsm_hooks.h. KRSI *is not that special*.

> Attaching BPF prog to one hook preserves zero cost for all other hooks.
> And when one hook is BPF powered it's using direct call instead of
> super expensive retpoline.

I'm not objecting to the good it does for KRSI.
I am *strongly* objecting to special casing KRSI.

> Overall this patch set looks good to me. There was a minor issue with prog
> accounting. I expect only that bit to be fixed in v5.
KP Singh Feb. 24, 2020, 5:13 p.m. UTC | #10
On 24-Feb 08:32, Casey Schaufler wrote:
> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
> > On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
> >> If I'm understanding this correctly, there are two issues:
> >>
> >> 1- BPF needs to be run last due to fexit trampolines (?)
> > no.
> > The placement of nop call can be anywhere.
> > BPF trampoline is automagically converting nop call into a sequence
> > of directly invoked BPF programs.
> > No link list traversals and no indirect calls in run-time.
> 
> Then why the insistence that it be last?

I think this came out of the discussion about not being able to
override the other LSMs and introduce a new attack vector with some
arguments discussed at:

  https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/

Let's say we have SELinux + BPF runnng on the system. BPF should still
respect any decisions made by SELinux. This hasn't got anything to
do with the usage of fexit trampolines.

> 
> >> 2- BPF hooks don't know what may be attached at any given time, so
> >>    ALL LSM hooks need to be universally hooked. THIS turns out to create
> >>    a measurable performance problem in that the cost of the indirect call
> >>    on the (mostly/usually) empty BPF policy is too high.
> > also no.
> 
> Um, then why not use the infrastructure as is?

I think what Kees meant is that BPF allows hooking to all the LSM
hooks and providing static LSM hook callbacks like traditional LSMs
has a measurable performance overhead and this is indeed correct. This
is why we want provide with the following characteristics:

- Without introducing a new attack surface, this was the reason for
  creating a mutable security_hook_heads in v1 which ran the hook
  after v1.

  This approach still had the issues of an indirect call and an
  extra check when not used. So this was not truly zero overhead even
  after special casing BPF.

- Having a truly zero performance overhead on the system. There are
  other tracing / attachment mechnaisms in the kernel which provide
  similar guarrantees (using static keys and direct calls) and it
  seems regressive for KRSI to not leverage these known patterns and
  sacrifice performance espeically in hotpaths. This provides users
  to use KRSI alongside other LSMs without paying extra cost for all
  the possible hooks.

> 
> >> So, trying to avoid the indirect calls is, as you say, an optimization,
> >> but it might be a needed one due to the other limitations.
> > I'm convinced that avoiding the cost of retpoline in critical path is a
> > requirement for any new infrastructure in the kernel.
> 
> Sorry, I haven't gotten that memo.
> 
> > Not only for security, but for any new infra.
> 
> The LSM infrastructure ain't new.

But the ability to attach BPF programs to LSM hooks is new. Also, we
have not had the whole implementation of the LSM hook be mutable
before and this is essentially what the KRSI provides.

> 
> > Networking stack converted all such places to conditional calls.
> > In BPF land we converted indirect calls to direct jumps and direct calls.
> > It took two years to do so. Adding new indirect calls is not an option.
> > I'm eagerly waiting for Peter's static_call patches to land to convert
> > a lot more indirect calls. May be existing LSMs will take advantage
> > of static_call patches too, but static_call is not an option for BPF.
> > That's why we introduced BPF trampoline in the last kernel release.
> 
> Sorry, but I don't see how BPF is so overwhelmingly special.

My analogy here is that if every tracepoint in the kernel were of the
type:

if (trace_foo_enabled) // <-- Overhead here, solved with static key
   trace_foo(a);  // <-- retpoline overhead, solved with fexit trampolines

It would be very hard to justify enabling them on a production system,
and the same can be said for BPF and KRSI.

- KP

> 
> >> b) Would there actually be a global benefit to using the static keys
> >>    optimization for other LSMs?
> > Yes. Just compiling with CONFIG_SECURITY adds "if (hlist_empty)" check
> > for every hook.
> 
> Err, no, it doesn't. It does an hlish_for_each_entry(), which
> may be the equivalent on an empty list, but let's not go around
> spreading misinformation.
> 
> >  Some of those hooks are in critical path. This load+cmp
> > can be avoided with static_key optimization. I think it's worth doing.
> 
> I admit to being unfamiliar with the static_key implementation,
> but if it would work for a list of hooks rather than a singe hook,
> I'm all ears.
> 
> >> If static keys are justified for KRSI
> > I really like that KRSI costs absolutely zero when it's not enabled.
> 
> And I dislike that there's security module specific code in security.c,
> security.h and/or lsm_hooks.h. KRSI *is not that special*.
> 
> > Attaching BPF prog to one hook preserves zero cost for all other hooks.
> > And when one hook is BPF powered it's using direct call instead of
> > super expensive retpoline.
> 
> I'm not objecting to the good it does for KRSI.
> I am *strongly* objecting to special casing KRSI.
> 
> > Overall this patch set looks good to me. There was a minor issue with prog
> > accounting. I expect only that bit to be fixed in v5.
>
KP Singh Feb. 24, 2020, 5:23 p.m. UTC | #11
Hi Kees,

Thanks for the feedback!

On 21-Feb 20:22, Kees Cook wrote:
> On Thu, Feb 20, 2020 at 03:49:05PM -0800, Casey Schaufler wrote:
> > On 2/20/2020 9:52 AM, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> > 
> > Sorry about the heavy list pruning - the original set
> > blows thunderbird up.
> 
> (I've added some people back; I had to dig this thread back out of lkml
> since I didn't get a direct copy...)
> 
> > > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > > overhead of retpolines. These programs cannot be attached to security_*
> > > wrappers as there are quite a few security_* functions that do more than
> > > just calling the LSM callbacks.
> > >
> > > This was discussed on the lists in:
> > >
> > >   https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> > >
> > > Adding a NOP callback after all the static LSM callbacks are called has
> > > the following benefits:
> > >
> > > - The BPF programs run at the right stage of the security_* wrappers.
> > > - They run after all the static LSM hooks allowed the operation,
> > >   therefore cannot allow an action that was already denied.
> > 
> > I still say that the special call-out to BPF is unnecessary.
> > I remain unconvinced by the arguments. You aren't doing anything
> > so special that the general mechanism won't work.
> 
> If I'm understanding this correctly, there are two issues:
> 
> 1- BPF needs to be run last due to fexit trampolines (?)
> 
> 2- BPF hooks don't know what may be attached at any given time, so
>    ALL LSM hooks need to be universally hooked. THIS turns out to create
>    a measurable performance problem in that the cost of the indirect call
>    on the (mostly/usually) empty BPF policy is too high.
> 
> "1" can be solved a lot of ways, and doesn't seem to be a debated part
> of this series.
> 
> "2" is interesting -- it creates a performance problem for EVERYONE that
> builds in this kernel feature, regardless of them using it. Excepting
> SELinux, "traditional" LSMs tends to be relatively sparse in their hooking:
> 
> $ grep '^      struct hlist_head' include/linux/lsm_hooks.h | wc -l
> 230
> $ for i in apparmor loadpin lockdown safesetid selinux smack tomoyo yama ; \
>   do echo -n "$i " && (cd $i && git grep LSM_HOOK_INIT | wc -l) ; done
> apparmor   68
> loadpin     3
> lockdown    1
> safesetid   2
> selinux   202
> smack     108
> tomoyo     28
> yama        4
> 
> So, trying to avoid the indirect calls is, as you say, an optimization,
> but it might be a needed one due to the other limitations.
> 
> To me, some questions present themselves:
> 
> a) What, exactly, are the performance characteristics of:
> 	"before"
> 	"with indirect calls"
> 	"with static keys optimization"

Good suggestion!

I will do some analysis and come back with the numbers.

> 
> b) Would there actually be a global benefit to using the static keys
>    optimization for other LSMs? (Especially given that they're already
>    sparsely populated and policy likely determines utility -- all the
>    LSMs would just turn ON all their static keys or turn off ALL their
>    static keys depending on having policy loaded.)

As Alexei mentioned, we can use the patches for static calls after
they are merged:

https://lore.kernel.org/lkml/8bc857824f82462a296a8a3c4913a11a7f801e74.1547073843.git.jpoimboe@redhat.com/

to make the framework better (as a separate series) especially given
that we are unsure how they work with BPF.

- KP

> 
> If static keys are justified for KRSI (by "a") then it seems the approach
> here should stand. If "b" is also true, then we need an additional
> series to apply this optimization for the other LSMs (but that seems
> distinctly separate from THIS series).
> 
> -- 
> Kees Cook
Casey Schaufler Feb. 24, 2020, 6:45 p.m. UTC | #12
On 2/24/2020 9:13 AM, KP Singh wrote:
> On 24-Feb 08:32, Casey Schaufler wrote:
>> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
>>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
>>>> If I'm understanding this correctly, there are two issues:
>>>>
>>>> 1- BPF needs to be run last due to fexit trampolines (?)
>>> no.
>>> The placement of nop call can be anywhere.
>>> BPF trampoline is automagically converting nop call into a sequence
>>> of directly invoked BPF programs.
>>> No link list traversals and no indirect calls in run-time.
>> Then why the insistence that it be last?
> I think this came out of the discussion about not being able to
> override the other LSMs and introduce a new attack vector with some
> arguments discussed at:
>
>   https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
>
> Let's say we have SELinux + BPF runnng on the system. BPF should still
> respect any decisions made by SELinux. This hasn't got anything to
> do with the usage of fexit trampolines.

The discussion sited is more about GPL than anything else.

The LSM rule is that any security module must be able to
accept the decisions of others. SELinux has to accept decisions
made ahead of it. It always has, as LSM checks occur after
"traditional" checks, which may fail. The only reason that you
need to be last in this implementation appears to be that you
refuse to use the general mechanisms. You can't blame SELinux
for that.

>>>> 2- BPF hooks don't know what may be attached at any given time, so
>>>>    ALL LSM hooks need to be universally hooked. THIS turns out to create
>>>>    a measurable performance problem in that the cost of the indirect call
>>>>    on the (mostly/usually) empty BPF policy is too high.
>>> also no.
>> Um, then why not use the infrastructure as is?
> I think what Kees meant is that BPF allows hooking to all the LSM
> hooks and providing static LSM hook callbacks like traditional LSMs
> has a measurable performance overhead and this is indeed correct. This
> is why we want provide with the following characteristics:

I was responding to the "also no", which denies both what Kees said
and what you're saying here. 

> - Without introducing a new attack surface, this was the reason for
>   creating a mutable security_hook_heads in v1 which ran the hook
>   after v1.

Yeah,

>   This approach still had the issues of an indirect call and an
>   extra check when not used. So this was not truly zero overhead even
>   after special casing BPF.

The LSM mechanism is not zero overhead. It never has been. That's why
you can compile it out. You get added value at a price. You get the
ability to use SELinux and KRSI together at a price. If that's unacceptable
you can go the route of seccomp, which doesn't use LSM for many of the
same reasons you're on about.

When LSM was introduced it was expected to be used by the lunatic fringe
people with government mandated security requirements. Today it has a
much greater general application. That's why I'm in the process of
bringing it up to modern use case requirements. Performance is much
more important now than it was before the use of LSM became popular.

> - Having a truly zero performance overhead on the system. There are
>   other tracing / attachment mechnaisms in the kernel which provide
>   similar guarrantees (using static keys and direct calls) and it
>   seems regressive for KRSI to not leverage these known patterns and
>   sacrifice performance espeically in hotpaths. This provides users
>   to use KRSI alongside other LSMs without paying extra cost for all
>   the possible hooks.

This is in direct conflict with the aforementioned "also no".

>>>> So, trying to avoid the indirect calls is, as you say, an optimization,
>>>> but it might be a needed one due to the other limitations.
>>> I'm convinced that avoiding the cost of retpoline in critical path is a
>>> requirement for any new infrastructure in the kernel.
>> Sorry, I haven't gotten that memo.
>>
>>> Not only for security, but for any new infra.
>> The LSM infrastructure ain't new.
> But the ability to attach BPF programs to LSM hooks is new.

Stop right there. No, I mean it. Really, stop right there.
I don't give a flying fig (he said, using the polite expression
rather than the vulgar) about what you want to do within a
security module. Attach a BPF program, randomize arbitrary
memory locations, do traditional Bell & LaPadula, it's all the
same to the LSM infrastructure. If you want to do something
that has to work outside that, the way audit and seccomp do,
you need to take that out of the LSM infrastructure. If you
want the convenience of the LSM infrastructure you don't get
to muck it up.

> Also, we
> have not had the whole implementation of the LSM hook be mutable
> before and this is essentially what the KRSI provides.

It can do that wholly within KRSI hooks. You don't need to
put KRSI specific code into security.c.

>>> Networking stack converted all such places to conditional calls.
>>> In BPF land we converted indirect calls to direct jumps and direct calls.
>>> It took two years to do so. Adding new indirect calls is not an option.
>>> I'm eagerly waiting for Peter's static_call patches to land to convert
>>> a lot more indirect calls. May be existing LSMs will take advantage
>>> of static_call patches too, but static_call is not an option for BPF.
>>> That's why we introduced BPF trampoline in the last kernel release.
>> Sorry, but I don't see how BPF is so overwhelmingly special.
> My analogy here is that if every tracepoint in the kernel were of the
> type:
>
> if (trace_foo_enabled) // <-- Overhead here, solved with static key
>    trace_foo(a);  // <-- retpoline overhead, solved with fexit trampolines
>
> It would be very hard to justify enabling them on a production system,
> and the same can be said for BPF and KRSI.

The same can be and has been said about the LSM infrastructure.
If BPF and KRSI are that performance critical you shouldn't be
tying them to LSM, which is known to have overhead. If you can't
accept the LSM overhead, get out of the LSM. Or, help us fix the
LSM infrastructure to make its overhead closer to zero. Whether
you believe it or not, a lot of work has gone into keeping the LSM
overhead as small as possible while remaining sufficiently general
to perform its function.

No. If you're too special to play by LSM rules then you're special
enough to get into the kernel using more direct means.
Kees Cook Feb. 24, 2020, 9:41 p.m. UTC | #13
On Mon, Feb 24, 2020 at 10:45:27AM -0800, Casey Schaufler wrote:
> On 2/24/2020 9:13 AM, KP Singh wrote:
> > On 24-Feb 08:32, Casey Schaufler wrote:
> >> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
> >>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
> >>>> If I'm understanding this correctly, there are two issues:
> >>>>
> >>>> 1- BPF needs to be run last due to fexit trampolines (?)
> >>> no.
> >>> The placement of nop call can be anywhere.
> >>> BPF trampoline is automagically converting nop call into a sequence
> >>> of directly invoked BPF programs.
> >>> No link list traversals and no indirect calls in run-time.
> >> Then why the insistence that it be last?
> > I think this came out of the discussion about not being able to
> > override the other LSMs and introduce a new attack vector with some
> > arguments discussed at:
> >
> >   https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
> >
> > Let's say we have SELinux + BPF runnng on the system. BPF should still
> > respect any decisions made by SELinux. This hasn't got anything to
> > do with the usage of fexit trampolines.
> 
> The discussion sited is more about GPL than anything else.
> 
> The LSM rule is that any security module must be able to
> accept the decisions of others. SELinux has to accept decisions
> made ahead of it. It always has, as LSM checks occur after
> "traditional" checks, which may fail. The only reason that you
> need to be last in this implementation appears to be that you
> refuse to use the general mechanisms. You can't blame SELinux
> for that.

Okay, this is why I wanted to try to state things plainly. The "in last
position" appears to be the result of a couple design choices:

-the idea of "not wanting to get in the way of other LSMs", while
 admirable, needs to actually be a non-goal to be "just" a stacked LSM
 (as you're saying here Casey). This position _was_ required for the
 non-privileged LSM case to avoid security implications, but that goal
 not longer exists here either.

-optimally using the zero-cost call-outs (static key + fexit trampolines)
 meant it didn't interact well with the existing stacking mechanism.

So, fine, these appear to be design choices, not *specifically*
requirements. Let's move on, I think there is more to unpack here...

> >>>> 2- BPF hooks don't know what may be attached at any given time, so
> >>>>    ALL LSM hooks need to be universally hooked. THIS turns out to create
> >>>>    a measurable performance problem in that the cost of the indirect call
> >>>>    on the (mostly/usually) empty BPF policy is too high.
> >>> also no.

AIUI, there was some confusion on Alexei's reply here. I, perhaps,
was not as clear as I needed to be. I think the later discussion on
performance overheads gets more into the point, and gets us closer to
the objections Alexei had. More below...

> >   This approach still had the issues of an indirect call and an
> >   extra check when not used. So this was not truly zero overhead even
> >   after special casing BPF.
> 
> The LSM mechanism is not zero overhead. It never has been. That's why
> you can compile it out. You get added value at a price. You get the
> ability to use SELinux and KRSI together at a price. If that's unacceptable
> you can go the route of seccomp, which doesn't use LSM for many of the
> same reasons you're on about.
> [...]
> >>>> So, trying to avoid the indirect calls is, as you say, an optimization,
> >>>> but it might be a needed one due to the other limitations.
> >>> I'm convinced that avoiding the cost of retpoline in critical path is a
> >>> requirement for any new infrastructure in the kernel.
> >> Sorry, I haven't gotten that memo.

I agree with Casey here -- it's a nice goal, but those cost evaluations have
not yet(?[1]) hit the LSM world. I think it's a desirable goal, to be
sure, but this does appear to be an early optimization.

> [...]
> It can do that wholly within KRSI hooks. You don't need to
> put KRSI specific code into security.c.

This observation is where I keep coming back to.

Yes, the resulting code is not as fast as it could be. The fact that BPF
triggers the worst-case performance of LSM hooking is the "new" part
here, from what I can see.

I suspect the problem is that folks in the BPF subsystem don't want to
be seen as slowing anything down, even other subsystems, so they don't
want to see this done in the traditional LSM hooking way (which contains
indirect calls).

But the LSM subsystem doesn't want special cases (Casey has worked very
hard to generalize everything there for stacking). It is really hard to
accept adding a new special case when there are still special cases yet
to be worked out even in the LSM code itself[2].

> >>> Networking stack converted all such places to conditional calls.
> >>> In BPF land we converted indirect calls to direct jumps and direct calls.
> >>> It took two years to do so. Adding new indirect calls is not an option.
> >>> I'm eagerly waiting for Peter's static_call patches to land to convert
> >>> a lot more indirect calls. May be existing LSMs will take advantage
> >>> of static_call patches too, but static_call is not an option for BPF.
> >>> That's why we introduced BPF trampoline in the last kernel release.
> >> Sorry, but I don't see how BPF is so overwhelmingly special.
> > My analogy here is that if every tracepoint in the kernel were of the
> > type:
> >
> > if (trace_foo_enabled) // <-- Overhead here, solved with static key
> >    trace_foo(a);  // <-- retpoline overhead, solved with fexit trampolines

This is a helpful distillation; thanks.

static keys (perhaps better described as static branches) make sense to
me; I'm familiar with them being used all over the place[3]. The resulting
"zero performance" branch mechanism is extremely handy.

I had been thinking about the fexit stuff only as a way for BPF to call
into kernel functions directly, and I missed the place where this got
used for calling from the kernel into BPF directly. KP walked me through
the fexit stuff off list. I missed where there NOP stub ("noinline int
bpf_lsm_##NAME(__VA_ARGS__) { return 0; }") was being patched by BPF in
https://lore.kernel.org/lkml/20200220175250.10795-6-kpsingh@chromium.org/
The key bit being "bpf_trampoline_update(prog)"

> > It would be very hard to justify enabling them on a production system,
> > and the same can be said for BPF and KRSI.
> 
> The same can be and has been said about the LSM infrastructure.
> If BPF and KRSI are that performance critical you shouldn't be
> tying them to LSM, which is known to have overhead. If you can't
> accept the LSM overhead, get out of the LSM. Or, help us fix the
> LSM infrastructure to make its overhead closer to zero. Whether
> you believe it or not, a lot of work has gone into keeping the LSM
> overhead as small as possible while remaining sufficiently general
> to perform its function.
> 
> No. If you're too special to play by LSM rules then you're special
> enough to get into the kernel using more direct means.

So, I see the primary conflict here being about the performance
optimizations. AIUI:

- BPF subsystem maintainers do not want any new slowdown associated
  with BPF
- LSM subsystem maintainers do not want any new special cases in
  LSM stacking

So, unless James is going to take this over Casey's objections, the path
forward I see here is:

- land a "slow" KRSI (i.e. one that hooks every hook with a stub).
- optimize calling for all LSMs

Does this seem right to everyone?

-Kees


[1] There is a "known cost to LSM", but as Casey mentions, it's been
generally deemed "acceptable". There have been some recent attempts to
quantify it, but it's not been very clear:
https://lore.kernel.org/linux-security-module/c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov/ (lore is missing half this conversation for some reason)

[2] Casey's work to generalize the LSM interfaces continues and it quite
complex:
https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/

[3] E.g. HARDENED_USERCOPY uses it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/usercopy.c?h=v5.5#n258
and so does the heap memory auto-initialization:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/slab.h?h=v5.5#n676
Casey Schaufler Feb. 24, 2020, 10:29 p.m. UTC | #14
On 2/24/2020 1:41 PM, Kees Cook wrote:
> On Mon, Feb 24, 2020 at 10:45:27AM -0800, Casey Schaufler wrote:
>> On 2/24/2020 9:13 AM, KP Singh wrote:
>>> On 24-Feb 08:32, Casey Schaufler wrote:
>>>> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
>>>>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
>>>>>> If I'm understanding this correctly, there are two issues:
>>>>>>
>>>>>> 1- BPF needs to be run last due to fexit trampolines (?)
>>>>> no.
>>>>> The placement of nop call can be anywhere.
>>>>> BPF trampoline is automagically converting nop call into a sequence
>>>>> of directly invoked BPF programs.
>>>>> No link list traversals and no indirect calls in run-time.
>>>> Then why the insistence that it be last?
>>> I think this came out of the discussion about not being able to
>>> override the other LSMs and introduce a new attack vector with some
>>> arguments discussed at:
>>>
>>>   https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
>>>
>>> Let's say we have SELinux + BPF runnng on the system. BPF should still
>>> respect any decisions made by SELinux. This hasn't got anything to
>>> do with the usage of fexit trampolines.
>> The discussion sited is more about GPL than anything else.
>>
>> The LSM rule is that any security module must be able to
>> accept the decisions of others. SELinux has to accept decisions
>> made ahead of it. It always has, as LSM checks occur after
>> "traditional" checks, which may fail. The only reason that you
>> need to be last in this implementation appears to be that you
>> refuse to use the general mechanisms. You can't blame SELinux
>> for that.
> Okay, this is why I wanted to try to state things plainly. The "in last
> position" appears to be the result of a couple design choices:
>
> -the idea of "not wanting to get in the way of other LSMs", while
>  admirable, needs to actually be a non-goal to be "just" a stacked LSM
>  (as you're saying here Casey). This position _was_ required for the
>  non-privileged LSM case to avoid security implications, but that goal
>  not longer exists here either.

Thanks.

> -optimally using the zero-cost call-outs (static key + fexit trampolines)
>  meant it didn't interact well with the existing stacking mechanism.

Exactly.

> So, fine, these appear to be design choices, not *specifically*
> requirements. Let's move on, I think there is more to unpack here...

Right.

>>>>>> 2- BPF hooks don't know what may be attached at any given time, so
>>>>>>    ALL LSM hooks need to be universally hooked. THIS turns out to create
>>>>>>    a measurable performance problem in that the cost of the indirect call
>>>>>>    on the (mostly/usually) empty BPF policy is too high.
>>>>> also no.
> AIUI, there was some confusion on Alexei's reply here. I, perhaps,
> was not as clear as I needed to be. I think the later discussion on
> performance overheads gets more into the point, and gets us closer to
> the objections Alexei had. More below...

Agreed.

>>>   This approach still had the issues of an indirect call and an
>>>   extra check when not used. So this was not truly zero overhead even
>>>   after special casing BPF.
>> The LSM mechanism is not zero overhead. It never has been. That's why
>> you can compile it out. You get added value at a price. You get the
>> ability to use SELinux and KRSI together at a price. If that's unacceptable
>> you can go the route of seccomp, which doesn't use LSM for many of the
>> same reasons you're on about.
>> [...]
>>>>>> So, trying to avoid the indirect calls is, as you say, an optimization,
>>>>>> but it might be a needed one due to the other limitations.
>>>>> I'm convinced that avoiding the cost of retpoline in critical path is a
>>>>> requirement for any new infrastructure in the kernel.
>>>> Sorry, I haven't gotten that memo.
> I agree with Casey here -- it's a nice goal, but those cost evaluations have
> not yet(?[1]) hit the LSM world. I think it's a desirable goal, to be
> sure, but this does appear to be an early optimization.

Thanks for helping clarify that.

>> [...]
>> It can do that wholly within KRSI hooks. You don't need to
>> put KRSI specific code into security.c.
> This observation is where I keep coming back to.
>
> Yes, the resulting code is not as fast as it could be. The fact that BPF
> triggers the worst-case performance of LSM hooking is the "new" part
> here, from what I can see.

I haven't put this oar in the water before, but mightn't it be
possible to configure which LSM hooks can have BPF programs installed
at compile time, and thus address this issue for the "production" case?
I fully expect that such a configuration option would be hideously ugly
both to implement and use. If the impact of unused BPF hooks is so
great a concern, perhaps it would be worthwhile.

> I suspect the problem is that folks in the BPF subsystem don't want to
> be seen as slowing anything down, even other subsystems, so they don't
> want to see this done in the traditional LSM hooking way (which contains
> indirect calls).

I get that, and it is a laudable goal, but ...

> But the LSM subsystem doesn't want special cases (Casey has worked very
> hard to generalize everything there for stacking). It is really hard to
> accept adding a new special case when there are still special cases yet
> to be worked out even in the LSM code itself[2].

... like Kees says, this isn't the only use case we have to deal with. 

>>>>> Networking stack converted all such places to conditional calls.
>>>>> In BPF land we converted indirect calls to direct jumps and direct calls.
>>>>> It took two years to do so. Adding new indirect calls is not an option.
>>>>> I'm eagerly waiting for Peter's static_call patches to land to convert
>>>>> a lot more indirect calls. May be existing LSMs will take advantage
>>>>> of static_call patches too, but static_call is not an option for BPF.
>>>>> That's why we introduced BPF trampoline in the last kernel release.
>>>> Sorry, but I don't see how BPF is so overwhelmingly special.
>>> My analogy here is that if every tracepoint in the kernel were of the
>>> type:
>>>
>>> if (trace_foo_enabled) // <-- Overhead here, solved with static key
>>>    trace_foo(a);  // <-- retpoline overhead, solved with fexit trampolines
> This is a helpful distillation; thanks.
>
> static keys (perhaps better described as static branches) make sense to
> me; I'm familiar with them being used all over the place[3]. The resulting
> "zero performance" branch mechanism is extremely handy.
>
> I had been thinking about the fexit stuff only as a way for BPF to call
> into kernel functions directly, and I missed the place where this got
> used for calling from the kernel into BPF directly. KP walked me through
> the fexit stuff off list. I missed where there NOP stub ("noinline int
> bpf_lsm_##NAME(__VA_ARGS__) { return 0; }") was being patched by BPF in
> https://lore.kernel.org/lkml/20200220175250.10795-6-kpsingh@chromium.org/
> The key bit being "bpf_trampoline_update(prog)"
>
>>> It would be very hard to justify enabling them on a production system,
>>> and the same can be said for BPF and KRSI.
>> The same can be and has been said about the LSM infrastructure.
>> If BPF and KRSI are that performance critical you shouldn't be
>> tying them to LSM, which is known to have overhead. If you can't
>> accept the LSM overhead, get out of the LSM. Or, help us fix the
>> LSM infrastructure to make its overhead closer to zero. Whether
>> you believe it or not, a lot of work has gone into keeping the LSM
>> overhead as small as possible while remaining sufficiently general
>> to perform its function.
>>
>> No. If you're too special to play by LSM rules then you're special
>> enough to get into the kernel using more direct means.
> So, I see the primary conflict here being about the performance
> optimizations. AIUI:
>
> - BPF subsystem maintainers do not want any new slowdown associated
>   with BPF

Right.

> - LSM subsystem maintainers do not want any new special cases in
>   LSM stacking

Right.

> So, unless James is going to take this over Casey's objections, the path
> forward I see here is:
>
> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> - optimize calling for all LSMs
>
> Does this seem right to everyone?

This would be my first choice. The existing list-of-hooks mechanism is
an "obvious" implementation. I have been thinking about possible ways to
make it better, but as y'all may have guessed, I haven't seen all the cool
new optimization techniques.

> -Kees
>
>
> [1] There is a "known cost to LSM", but as Casey mentions, it's been
> generally deemed "acceptable". There have been some recent attempts to
> quantify it, but it's not been very clear:
> https://lore.kernel.org/linux-security-module/c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov/ (lore is missing half this conversation for some reason)
>
> [2] Casey's work to generalize the LSM interfaces continues and it quite
> complex:
> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
>
> [3] E.g. HARDENED_USERCOPY uses it:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/usercopy.c?h=v5.5#n258
> and so does the heap memory auto-initialization:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/slab.h?h=v5.5#n676
>
Alexei Starovoitov Feb. 25, 2020, 5:41 a.m. UTC | #15
On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
> 
> But the LSM subsystem doesn't want special cases (Casey has worked very
> hard to generalize everything there for stacking). It is really hard to
> accept adding a new special case when there are still special cases yet
> to be worked out even in the LSM code itself[2].
> [2] Casey's work to generalize the LSM interfaces continues and it quite
> complex:
> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/

I think the key mistake we made is that we classified KRSI as LSM.
LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
I don't see anything in LSM infra that KRSI can reuse.
The only thing BPF needs is a function to attach to.
It can be a nop function or any other.
security_*() functions are interesting from that angle only.
Hence I propose to reconsider what I was suggesting earlier.
No changes to secruity/ directory.
Attach to security_*() funcs via bpf trampoline.
The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
I think "security" is also loaded word that should be avoided.
I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.

> So, unless James is going to take this over Casey's objections, the path
> forward I see here is:
> 
> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> - optimize calling for all LSMs

I'm very much surprised how 'slow' KRSI is an option at all.
'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
functions for every place in the kernel that calls security_*().
This is not an acceptable overhead. Even w/o retpoline
this is not something datacenter servers can use.

Another option is to do this:
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..7887ce636fb1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
        return kernel_load_data_str[id];
 }

-#ifdef CONFIG_SECURITY
+#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)

Single line change to security.h and new file kernel/bpf/override_security.c
that will look like:
int security_binder_set_context_mgr(struct task_struct *mgr)
{
        return 0;
}

int security_binder_transaction(struct task_struct *from,
                                struct task_struct *to)
{
        return 0;
}
Essentially it will provide BPF side with a set of nop functions.
CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
on kernel users. Either they build the kernel with CONFIG_SECURITY and their
choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
it's a pro not a con.
Kees Cook Feb. 25, 2020, 3:31 p.m. UTC | #16
On Mon, Feb 24, 2020 at 09:41:27PM -0800, Alexei Starovoitov wrote:
> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.

Isn't the type used to decide which validator to use?
KP Singh Feb. 25, 2020, 7:29 p.m. UTC | #17
On 24-Feb 13:41, Kees Cook wrote:
> On Mon, Feb 24, 2020 at 10:45:27AM -0800, Casey Schaufler wrote:
> > On 2/24/2020 9:13 AM, KP Singh wrote:
> > > On 24-Feb 08:32, Casey Schaufler wrote:
> > >> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
> > >>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
> > >>>> If I'm understanding this correctly, there are two issues:
> > >>>>
> > >>>> 1- BPF needs to be run last due to fexit trampolines (?)
> > >>> no.
> > >>> The placement of nop call can be anywhere.
> > >>> BPF trampoline is automagically converting nop call into a sequence
> > >>> of directly invoked BPF programs.
> > >>> No link list traversals and no indirect calls in run-time.
> > >> Then why the insistence that it be last?
> > > I think this came out of the discussion about not being able to
> > > override the other LSMs and introduce a new attack vector with some
> > > arguments discussed at:
> > >
> > >   https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
> > >
> > > Let's say we have SELinux + BPF runnng on the system. BPF should still
> > > respect any decisions made by SELinux. This hasn't got anything to
> > > do with the usage of fexit trampolines.
> > 
> > The discussion sited is more about GPL than anything else.
> > 
> > The LSM rule is that any security module must be able to
> > accept the decisions of others. SELinux has to accept decisions
> > made ahead of it. It always has, as LSM checks occur after
> > "traditional" checks, which may fail. The only reason that you
> > need to be last in this implementation appears to be that you
> > refuse to use the general mechanisms. You can't blame SELinux
> > for that.
> 
> Okay, this is why I wanted to try to state things plainly. The "in last
> position" appears to be the result of a couple design choices:
> 
> -the idea of "not wanting to get in the way of other LSMs", while
>  admirable, needs to actually be a non-goal to be "just" a stacked LSM
>  (as you're saying here Casey). This position _was_ required for the
>  non-privileged LSM case to avoid security implications, but that goal
>  not longer exists here either.
> 
> -optimally using the zero-cost call-outs (static key + fexit trampolines)
>  meant it didn't interact well with the existing stacking mechanism.
> 
> So, fine, these appear to be design choices, not *specifically*
> requirements. Let's move on, I think there is more to unpack here...
> 
> > >>>> 2- BPF hooks don't know what may be attached at any given time, so
> > >>>>    ALL LSM hooks need to be universally hooked. THIS turns out to create
> > >>>>    a measurable performance problem in that the cost of the indirect call
> > >>>>    on the (mostly/usually) empty BPF policy is too high.
> > >>> also no.
> 
> AIUI, there was some confusion on Alexei's reply here. I, perhaps,
> was not as clear as I needed to be. I think the later discussion on
> performance overheads gets more into the point, and gets us closer to
> the objections Alexei had. More below...
> 
> > >   This approach still had the issues of an indirect call and an
> > >   extra check when not used. So this was not truly zero overhead even
> > >   after special casing BPF.
> > 
> > The LSM mechanism is not zero overhead. It never has been. That's why
> > you can compile it out. You get added value at a price. You get the
> > ability to use SELinux and KRSI together at a price. If that's unacceptable
> > you can go the route of seccomp, which doesn't use LSM for many of the
> > same reasons you're on about.
> > [...]
> > >>>> So, trying to avoid the indirect calls is, as you say, an optimization,
> > >>>> but it might be a needed one due to the other limitations.
> > >>> I'm convinced that avoiding the cost of retpoline in critical path is a
> > >>> requirement for any new infrastructure in the kernel.
> > >> Sorry, I haven't gotten that memo.
> 
> I agree with Casey here -- it's a nice goal, but those cost evaluations have
> not yet(?[1]) hit the LSM world. I think it's a desirable goal, to be
> sure, but this does appear to be an early optimization.
> 
> > [...]
> > It can do that wholly within KRSI hooks. You don't need to
> > put KRSI specific code into security.c.
> 
> This observation is where I keep coming back to.
> 
> Yes, the resulting code is not as fast as it could be. The fact that BPF
> triggers the worst-case performance of LSM hooking is the "new" part
> here, from what I can see.
> 
> I suspect the problem is that folks in the BPF subsystem don't want to
> be seen as slowing anything down, even other subsystems, so they don't
> want to see this done in the traditional LSM hooking way (which contains
> indirect calls).
> 
> But the LSM subsystem doesn't want special cases (Casey has worked very
> hard to generalize everything there for stacking). It is really hard to
> accept adding a new special case when there are still special cases yet
> to be worked out even in the LSM code itself[2].
> 
> > >>> Networking stack converted all such places to conditional calls.
> > >>> In BPF land we converted indirect calls to direct jumps and direct calls.
> > >>> It took two years to do so. Adding new indirect calls is not an option.
> > >>> I'm eagerly waiting for Peter's static_call patches to land to convert
> > >>> a lot more indirect calls. May be existing LSMs will take advantage
> > >>> of static_call patches too, but static_call is not an option for BPF.
> > >>> That's why we introduced BPF trampoline in the last kernel release.
> > >> Sorry, but I don't see how BPF is so overwhelmingly special.
> > > My analogy here is that if every tracepoint in the kernel were of the
> > > type:
> > >
> > > if (trace_foo_enabled) // <-- Overhead here, solved with static key
> > >    trace_foo(a);  // <-- retpoline overhead, solved with fexit trampolines
> 
> This is a helpful distillation; thanks.
> 
> static keys (perhaps better described as static branches) make sense to
> me; I'm familiar with them being used all over the place[3]. The resulting
> "zero performance" branch mechanism is extremely handy.
> 
> I had been thinking about the fexit stuff only as a way for BPF to call
> into kernel functions directly, and I missed the place where this got
> used for calling from the kernel into BPF directly. KP walked me through
> the fexit stuff off list. I missed where there NOP stub ("noinline int
> bpf_lsm_##NAME(__VA_ARGS__) { return 0; }") was being patched by BPF in
> https://lore.kernel.org/lkml/20200220175250.10795-6-kpsingh@chromium.org/
> The key bit being "bpf_trampoline_update(prog)"
> 
> > > It would be very hard to justify enabling them on a production system,
> > > and the same can be said for BPF and KRSI.
> > 
> > The same can be and has been said about the LSM infrastructure.
> > If BPF and KRSI are that performance critical you shouldn't be
> > tying them to LSM, which is known to have overhead. If you can't
> > accept the LSM overhead, get out of the LSM. Or, help us fix the
> > LSM infrastructure to make its overhead closer to zero. Whether
> > you believe it or not, a lot of work has gone into keeping the LSM
> > overhead as small as possible while remaining sufficiently general
> > to perform its function.
> > 
> > No. If you're too special to play by LSM rules then you're special
> > enough to get into the kernel using more direct means.
> 
> So, I see the primary conflict here being about the performance
> optimizations. AIUI:
> 
> - BPF subsystem maintainers do not want any new slowdown associated
>   with BPF
> - LSM subsystem maintainers do not want any new special cases in
>   LSM stacking
> 
> So, unless James is going to take this over Casey's objections, the path
> forward I see here is:
> 
> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> - optimize calling for all LSMs

I will work on v5 which resgisters the nops as standard LSM hooks and
we can follow-up on performance.

- KP

> 
> Does this seem right to everyone?
> 
> -Kees
> 
> 
> [1] There is a "known cost to LSM", but as Casey mentions, it's been
> generally deemed "acceptable". There have been some recent attempts to
> quantify it, but it's not been very clear:
> https://lore.kernel.org/linux-security-module/c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov/ (lore is missing half this conversation for some reason)
> 
> [2] Casey's work to generalize the LSM interfaces continues and it quite
> complex:
> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
> 
> [3] E.g. HARDENED_USERCOPY uses it:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/usercopy.c?h=v5.5#n258
> and so does the heap memory auto-initialization:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/slab.h?h=v5.5#n676
> 
> -- 
> Kees Cook
KP Singh Feb. 25, 2020, 7:31 p.m. UTC | #18
On 24-Feb 21:41, Alexei Starovoitov wrote:
> On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
> > 
> > But the LSM subsystem doesn't want special cases (Casey has worked very
> > hard to generalize everything there for stacking). It is really hard to
> > accept adding a new special case when there are still special cases yet
> > to be worked out even in the LSM code itself[2].
> > [2] Casey's work to generalize the LSM interfaces continues and it quite
> > complex:
> > https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
> 
> I think the key mistake we made is that we classified KRSI as LSM.
> LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
> I don't see anything in LSM infra that KRSI can reuse.
> The only thing BPF needs is a function to attach to.
> It can be a nop function or any other.
> security_*() functions are interesting from that angle only.
> Hence I propose to reconsider what I was suggesting earlier.
> No changes to secruity/ directory.
> Attach to security_*() funcs via bpf trampoline.
> The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
> I think "security" is also loaded word that should be avoided.
> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.

The BPF_PROG_TYPE_OVERRIDE_RETURN seems to be useful in general as
well and we have the implementation already figured out as a part of
the LSM work. I will split that bit into a separate series.

- KP

> 
> > So, unless James is going to take this over Casey's objections, the path
> > forward I see here is:
> > 
> > - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> > - optimize calling for all LSMs
> 
> I'm very much surprised how 'slow' KRSI is an option at all.
> 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
> functions for every place in the kernel that calls security_*().
> This is not an acceptable overhead. Even w/o retpoline
> this is not something datacenter servers can use.
> 
> Another option is to do this:
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 64b19f050343..7887ce636fb1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>         return kernel_load_data_str[id];
>  }
> 
> -#ifdef CONFIG_SECURITY
> +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
> 
> Single line change to security.h and new file kernel/bpf/override_security.c
> that will look like:
> int security_binder_set_context_mgr(struct task_struct *mgr)
> {
>         return 0;
> }
> 
> int security_binder_transaction(struct task_struct *from,
>                                 struct task_struct *to)
> {
>         return 0;
> }
> Essentially it will provide BPF side with a set of nop functions.
> CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
> on kernel users. Either they build the kernel with CONFIG_SECURITY and their
> choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
> BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
> it's a pro not a con.
Casey Schaufler Feb. 26, 2020, 12:30 a.m. UTC | #19
On 2/24/2020 9:41 PM, Alexei Starovoitov wrote:
> On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
>> But the LSM subsystem doesn't want special cases (Casey has worked very
>> hard to generalize everything there for stacking). It is really hard to
>> accept adding a new special case when there are still special cases yet
>> to be worked out even in the LSM code itself[2].
>> [2] Casey's work to generalize the LSM interfaces continues and it quite
>> complex:
>> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
> I think the key mistake we made is that we classified KRSI as LSM.
> LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
> I don't see anything in LSM infra that KRSI can reuse.
> The only thing BPF needs is a function to attach to.
> It can be a nop function or any other.
> security_*() functions are interesting from that angle only.
> Hence I propose to reconsider what I was suggesting earlier.
> No changes to secruity/ directory.
> Attach to security_*() funcs via bpf trampoline.
> The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
> I think "security" is also loaded word that should be avoided.

No argument there.

> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
>
>> So, unless James is going to take this over Casey's objections, the path
>> forward I see here is:
>>
>> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
>> - optimize calling for all LSMs
> I'm very much surprised how 'slow' KRSI is an option at all.
> 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
> functions for every place in the kernel that calls security_*().
> This is not an acceptable overhead. Even w/o retpoline
> this is not something datacenter servers can use.

In the universe I live in data centers will disable hyper-threading,
reducing performance substantially, in the face of hypothetical security
exploits. That's a massively greater performance impact than the handful
of instructions required to do indirect calls. Not to mention the impact
of the BPF programs that have been included. Have you ever looked at what
happens to system performance when polkitd is enabled?


>
> Another option is to do this:
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 64b19f050343..7887ce636fb1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>         return kernel_load_data_str[id];
>  }
>
> -#ifdef CONFIG_SECURITY
> +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
>
> Single line change to security.h and new file kernel/bpf/override_security.c
> that will look like:
> int security_binder_set_context_mgr(struct task_struct *mgr)
> {
>         return 0;
> }
>
> int security_binder_transaction(struct task_struct *from,
>                                 struct task_struct *to)
> {
>         return 0;
> }
> Essentially it will provide BPF side with a set of nop functions.
> CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
> on kernel users. Either they build the kernel with CONFIG_SECURITY and their
> choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
> BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
> it's a pro not a con.

Err, no. All distros use an LSM or two. Unless you can re-implement SELinux
in BPF (good luck with state transitions) you've built a warp drive without
ever having mined dilithium crystals.
KP Singh Feb. 26, 2020, 5:15 a.m. UTC | #20
On 25-Feb 16:30, Casey Schaufler wrote:
> On 2/24/2020 9:41 PM, Alexei Starovoitov wrote:
> > On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
> >> But the LSM subsystem doesn't want special cases (Casey has worked very
> >> hard to generalize everything there for stacking). It is really hard to
> >> accept adding a new special case when there are still special cases yet
> >> to be worked out even in the LSM code itself[2].
> >> [2] Casey's work to generalize the LSM interfaces continues and it quite
> >> complex:
> >> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
> > I think the key mistake we made is that we classified KRSI as LSM.
> > LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
> > I don't see anything in LSM infra that KRSI can reuse.
> > The only thing BPF needs is a function to attach to.
> > It can be a nop function or any other.
> > security_*() functions are interesting from that angle only.
> > Hence I propose to reconsider what I was suggesting earlier.
> > No changes to secruity/ directory.
> > Attach to security_*() funcs via bpf trampoline.
> > The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
> > I think "security" is also loaded word that should be avoided.
> 
> No argument there.
> 
> > I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
> >
> >> So, unless James is going to take this over Casey's objections, the path
> >> forward I see here is:
> >>
> >> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> >> - optimize calling for all LSMs
> > I'm very much surprised how 'slow' KRSI is an option at all.
> > 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
> > functions for every place in the kernel that calls security_*().
> > This is not an acceptable overhead. Even w/o retpoline
> > this is not something datacenter servers can use.
> 
> In the universe I live in data centers will disable hyper-threading,
> reducing performance substantially, in the face of hypothetical security
> exploits. That's a massively greater performance impact than the handful
> of instructions required to do indirect calls. Not to mention the impact

Indirect calls have worse performance implications than just a few
instructions and are especially not suitable for hotpaths.

There have been multiple efforts to reduce their usage e.g.:

  - https://lwn.net/Articles/774743/
  - https://lwn.net/Articles/773985/

> of the BPF programs that have been included. Have you ever looked at what

  BPF programs are JIT'ed and optimized to native code.

> happens to system performance when polkitd is enabled?

However, let's discuss all this separately when we follow-up with
performance improvements after submitting the initial patch-set.

> 
> 
> >
> > Another option is to do this:
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 64b19f050343..7887ce636fb1 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
> >         return kernel_load_data_str[id];
> >  }
> >
> > -#ifdef CONFIG_SECURITY
> > +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
> >
> > Single line change to security.h and new file kernel/bpf/override_security.c
> > that will look like:
> > int security_binder_set_context_mgr(struct task_struct *mgr)
> > {
> >         return 0;
> > }
> >
> > int security_binder_transaction(struct task_struct *from,
> >                                 struct task_struct *to)
> > {
> >         return 0;
> > }
> > Essentially it will provide BPF side with a set of nop functions.
> > CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
> > on kernel users. Either they build the kernel with CONFIG_SECURITY and their
> > choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
> > BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
> > it's a pro not a con.
> 
> Err, no. All distros use an LSM or two. Unless you can re-implement SELinux

The users mentioned here in this context are (I would assume) the more
performance sensitive users who would, potentially, disable
CONFIG_SECURITY because of the current performance characteristics.

We can also discuss this separately and only if we find that we need
it for the BPF_OVERRIDE_RET type attachment.

- KP

> in BPF (good luck with state transitions) you've built a warp drive without
> ever having mined dilithium crystals.
> 
>
Casey Schaufler Feb. 26, 2020, 3:35 p.m. UTC | #21
On 2/25/2020 9:15 PM, KP Singh wrote:
> On 25-Feb 16:30, Casey Schaufler wrote:
>> On 2/24/2020 9:41 PM, Alexei Starovoitov wrote:
>>> On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
>>>> But the LSM subsystem doesn't want special cases (Casey has worked very
>>>> hard to generalize everything there for stacking). It is really hard to
>>>> accept adding a new special case when there are still special cases yet
>>>> to be worked out even in the LSM code itself[2].
>>>> [2] Casey's work to generalize the LSM interfaces continues and it quite
>>>> complex:
>>>> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
>>> I think the key mistake we made is that we classified KRSI as LSM.
>>> LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
>>> I don't see anything in LSM infra that KRSI can reuse.
>>> The only thing BPF needs is a function to attach to.
>>> It can be a nop function or any other.
>>> security_*() functions are interesting from that angle only.
>>> Hence I propose to reconsider what I was suggesting earlier.
>>> No changes to secruity/ directory.
>>> Attach to security_*() funcs via bpf trampoline.
>>> The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
>>> I think "security" is also loaded word that should be avoided.
>> No argument there.
>>
>>> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
>>>
>>>> So, unless James is going to take this over Casey's objections, the path
>>>> forward I see here is:
>>>>
>>>> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
>>>> - optimize calling for all LSMs
>>> I'm very much surprised how 'slow' KRSI is an option at all.
>>> 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
>>> functions for every place in the kernel that calls security_*().
>>> This is not an acceptable overhead. Even w/o retpoline
>>> this is not something datacenter servers can use.
>> In the universe I live in data centers will disable hyper-threading,
>> reducing performance substantially, in the face of hypothetical security
>> exploits. That's a massively greater performance impact than the handful
>> of instructions required to do indirect calls. Not to mention the impact
> Indirect calls have worse performance implications than just a few
> instructions and are especially not suitable for hotpaths.
>
> There have been multiple efforts to reduce their usage e.g.:
>
>   - https://lwn.net/Articles/774743/
>   - https://lwn.net/Articles/773985/
>
>> of the BPF programs that have been included. Have you ever looked at what
>   BPF programs are JIT'ed and optimized to native code.

Doesn't mean people won't write slow code.


>> happens to system performance when polkitd is enabled?
> However, let's discuss all this separately when we follow-up with
> performance improvements after submitting the initial patch-set.

Think performance up front. Don't ignore issues.

>>> Another option is to do this:
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 64b19f050343..7887ce636fb1 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>>>         return kernel_load_data_str[id];
>>>  }
>>>
>>> -#ifdef CONFIG_SECURITY
>>> +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
>>>
>>> Single line change to security.h and new file kernel/bpf/override_security.c
>>> that will look like:
>>> int security_binder_set_context_mgr(struct task_struct *mgr)
>>> {
>>>         return 0;
>>> }
>>>
>>> int security_binder_transaction(struct task_struct *from,
>>>                                 struct task_struct *to)
>>> {
>>>         return 0;
>>> }
>>> Essentially it will provide BPF side with a set of nop functions.
>>> CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
>>> on kernel users. Either they build the kernel with CONFIG_SECURITY and their
>>> choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
>>> BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
>>> it's a pro not a con.
>> Err, no. All distros use an LSM or two. Unless you can re-implement SELinux
> The users mentioned here in this context are (I would assume) the more
> performance sensitive users who would, potentially, disable
> CONFIG_SECURITY because of the current performance characteristics.

You assume that the most performance sensitive people would allow
a mechanism to arbitrarily add overhead that is out of their control?
How does that make sense?

> We can also discuss this separately and only if we find that we need
> it for the BPF_OVERRIDE_RET type attachment.
>
> - KP
>
>> in BPF (good luck with state transitions) you've built a warp drive without
>> ever having mined dilithium crystals.
>>
>>
diff mbox series

Patch

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
new file mode 100644
index 000000000000..f867f72f6aa9
--- /dev/null
+++ b/include/linux/bpf_lsm.h
@@ -0,0 +1,34 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#ifndef _LINUX_BPF_LSM_H
+#define _LINUX_BPF_LSM_H
+
+#include <linux/bpf.h>
+
+#ifdef CONFIG_BPF_LSM
+
+#define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__);
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK
+
+#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) bpf_lsm_##FUNC(__VA_ARGS__)
+#define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) ({				\
+	do {								\
+		if (RC == 0)						\
+			RC = bpf_lsm_##FUNC(__VA_ARGS__);		\
+	} while (0);							\
+	RC;								\
+})
+
+#else /* !CONFIG_BPF_LSM */
+
+#define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) (RC)
+#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...)
+
+#endif /* CONFIG_BPF_LSM */
+
+#endif /* _LINUX_BPF_LSM_H */
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index affb6941622e..abc847c9b9a1 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -7,6 +7,22 @@ 
 #include <linux/filter.h>
 #include <linux/bpf.h>
 #include <linux/btf.h>
+#include <linux/bpf_lsm.h>
+
+/* For every LSM hook  that allows attachment of BPF programs, declare a NOP
+ * function where a BPF program can be attached as an fexit trampoline.
+ */
+#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
+#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__)  \
+{									  \
+	return 0;							  \
+}
+
+#define LSM_HOOK_void(NAME, ...) \
+	noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
+
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK
 
 const struct bpf_prog_ops lsm_prog_ops = {
 };
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..aa111392a700 100644
--- a/security/security.c
+++ b/security/security.c
@@ -28,6 +28,7 @@ 
 #include <linux/string.h>
 #include <linux/msg.h>
 #include <net/flow.h>
+#include <linux/bpf_lsm.h>
 
 #define MAX_LSM_EVM_XATTR	2
 
@@ -684,6 +685,7 @@  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__);		\
+		RUN_BPF_LSM_VOID_PROGS(FUNC, __VA_ARGS__);	\
 	} while (0)
 
 #define call_int_hook(FUNC, IRC, ...) ({			\
@@ -696,6 +698,7 @@  static void __init lsm_early_task(struct task_struct *task)
 			if (RC != 0)				\
 				break;				\
 		}						\
+		RC = RUN_BPF_LSM_INT_PROGS(RC, FUNC, __VA_ARGS__); \
 	} while (0);						\
 	RC;							\
 })