Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)

From: Casey Schaufler <casey@schaufler-ca.com>
To: Kees Cook <keescook@chromium.org>
Cc: KP Singh <kpsingh@chromium.org>,
	Linux Security Module list 
	<linux-security-module@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	James Morris <jmorris@namei.org>,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
Date: Fri, 21 Feb 2020 17:04:38 -0800	[thread overview]
Message-ID: <7fd415e0-35c8-e30e-e4b8-af0ba286f628@schaufler-ca.com> (raw)
In-Reply-To: <202002211617.28EAC6826@keescook>

On 2/21/2020 4:22 PM, Kees Cook wrote:
> On Fri, Feb 21, 2020 at 02:31:18PM -0800, Casey Schaufler wrote:
>> On 2/21/2020 11:41 AM, KP Singh wrote:
>>> On 21-Feb 11:19, Casey Schaufler wrote:
>>>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>>>> From: KP Singh <kpsingh@google.com>
>>>>> # v3 -> v4
>>>>>
>>>>>   https://lkml.org/lkml/2020/1/23/515
>>>>>
>>>>> * Moved away from allocating a separate security_hook_heads and adding a
>>>>>   new special case for arch_prepare_bpf_trampoline to using BPF fexit
>>>>>   trampolines called from the right place in the LSM hook and toggled by
>>>>>   static keys based on the discussion in:
>>>>>
>>>>>     https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
>>>>>
>>>>> * Since the code does not deal with security_hook_heads anymore, it goes
>>>>>   from "being a BPF LSM" to "BPF program attachment to LSM hooks".
>>>> I've finally been able to review the entire patch set.
>>>> I can't imagine how it can make sense to add this much
>>>> complexity to the LSM infrastructure in support of this
>>>> feature. There is macro magic going on that is going to
>>>> break, and soon. You are introducing dependencies on BPF
>>>> into the infrastructure, and that's unnecessary and most
>>>> likely harmful.
>>> We will be happy to document each of the macros in detail. Do note a
>>> few things here:
>>>
>>> * There is really nothing magical about them though,
>>
>> +#define LSM_HOOK_void(NAME, ...) \
>> +	noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
>> +
>> +#include <linux/lsm_hook_names.h>
>> +#undef LSM_HOOK
>>
>> I haven't seen anything this ... novel ... in a very long time.
>> I see why you want to do this, but you're tying the two sets
>> of code together unnaturally. When (not if) the two sets diverge
>> you're going to be introducing another clever way to deal with
>> the special case.
> I really like this approach: it actually _simplifies_ the LSM piece in
> that there is no need to keep the union and the hook lists in sync any
> more: they're defined once now. (There were already 2 lists, and this
> collapses the list into 1 place for all 3 users.) It's very visible in
> the diffstat too (~300 lines removed):

Erk. Too many smart people like this. I still don't, but it's possible
that I could learn to.

>
>  include/linux/lsm_hook_names.h | 353 +++++++++++++++++++
>  include/linux/lsm_hooks.h      | 622 +--------------------------------
>  2 files changed, 359 insertions(+), 616 deletions(-)
>
> Also, there is no need to worry about divergence: the BPF will always
> track the exposed LSM. Backward compat is (AIUI) explicitly a
> non-feature.

As written you're correct, it can't diverge. My concern is about
what happens when someone decides that they want the BPF and hook
to be different. I fear there will be a hideous solution.

> I don't see why anything here is "harmful"?

Injecting large chucks of code via an #include does nothing
for readability. I've seen it fail disastrously many times,
usually after the original author has moved on and entrusted
the code to someone who missed some of the nuance.

I'll drop objection to this bit, but still object to making
BPF special in the infrastructure. It doesn't need to be and
it is exactly the kind of additional complexity we need to
avoid.