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

From: Casey Schaufler
Date: Fri Feb 21 2020 - 18:49:19 EST

On 2/21/2020 3:09 PM, KP Singh wrote:
> Thanks Casey,
> I appreciate your quick responses!
> On 21-Feb 14:31, 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@xxxxxxxxxx>
>>>> Again, apologies for the CC list trimming.
>>>>> # v3 -> v4
>>>>> * 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:
>>>>> * 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".
> [...]
>>>> 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.
> This is not "novel", it's a fairly common pattern followed in tracing:
> For example, the TRACE_INCLUDE macro which is used for tracepoints:
> Â include/trace/define_trace.h
> and used in:
> Â * include/trace/bpf_probe.h
> Â Â
> Â * include/trace/perf.h
> Â Â
> Â * include/trace/trace_events.h

I can't say I care for that, either, and it's a simpler case.

>> 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
> I don't fully understand what "two sets diverge means" here. All the
> BPF headers need is the name, return type and the args. This is the
> same information which is needed by the call_{int, void}_hooks and the
> LSM declarataions (i.e. security_hook_heads and
> security_list_options).

As you've noticed, not all the interfaces can use call_{int,void}_hooks.
If you've been following the stacking efforts, you'll see that increasing.

At some point I anticipate a BPF hook that needs different information
than the LSM hook. That's been discussed, too. Asserting that it will
never happen does not make me comfortable.

>> the special case.
>> It's not that I don't understand what you're doing. It's that
>> I don't like what you're doing. Explanation doesn't make me like
>> it better.
> As I have previously said, we will be happy to (and have already)
> updated our approach based on the consensus we arrive at here.

Not to put too fine a point on it, but I have raised the same
objection - that you should use the infrastructure as it is -
each time. I do not see consensus, I see you plowing ahead with
the direction you've chosen in spite of the significant objection.

> The
> best outcome would be to not sacrifice performance as the LSM hooks
> are called from various performance critical code-paths.

Then help me tune the infrastructure to be better in those cases.

> It would be great to know the maintainers' (BPF and Security)
> perspective on this as well.

Many eyes and all that, but the BPF maintainers haven't been working
with the LSM infrastructure and won't be familiar with its quirks.