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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c5c67ece-e5c1-9e8f-3a2b-60d8d002c894@schaufler-ca.com>
Date:   Mon, 24 Feb 2020 08:32:10 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Kees Cook <keescook@...omium.org>
Cc:     KP Singh <kpsingh@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        James Morris <jmorris@...ei.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for
 BPF LSM programs

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.