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

[Casey Schaufler]

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@xxxxxxxxxx>
>> 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@xxxxxxxxxxxx/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).
>