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

[Casey Schaufler <casey@schaufler-ca.com>]

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@google.com>
>>>> Again, apologies for the CC list trimming.
>>>>
>>>>> # 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".
> [...]
>
>>>> 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
>
>     https://github.com/torvalds/linux/blob/master/include/trace/bpf_probe.h#L110
>
>   * include/trace/perf.h
>
>     https://github.com/torvalds/linux/blob/master/include/trace/perf.h#L90
>
>   * include/trace/trace_events.h
>
>     https://github.com/torvalds/linux/blob/master/include/trace/trace_events.h#L402

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.