Message-ID: <202002211909.10D57A125@keescook>
Date:   Fri, 21 Feb 2020 19:36:06 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     KP Singh <kpsingh@...omium.org>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        James Morris <jmorris@...ei.org>
Subject: Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)

On Fri, Feb 21, 2020 at 05:04:38PM -0800, Casey Schaufler wrote:
> On 2/21/2020 4:22 PM, Kees Cook wrote:
> > 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.

Well, I admit that I am, perhaps, overly infatuatied with "fancy" macros,
but in cases like this where we're operating on a list of stuff and doing
the same thing over and over but with different elements, I've found
this is actually much nicer way to do it. (E.g. I did something like
this in drivers/misc/lkdtm/core.c to avoid endless typing, and Mimi did
something similar in include/linux/fs.h for keeping kernel_read_file_id
and kernel_read_file_str automatically in sync.) KP's macros are more
extensive, but I think it's a clever to avoid going crazy as LSM hooks
evolve.

> > 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.

This is related to some of the discussion at the last Maintainer's
Summit and tracepoints: i.e. the exposure of what is basically kernel
internals to a userspace system. The conclusion there (which, I think,
has been extended strongly into BPF) is that things that produce BPF are
accepted to be strongly tied to kernel version, so if a hook changes, so
much the userspace side. This appears to be proven out in the existing
BPF world, which gives me some evidence that this claim (close tie to
kernel version) isn't an empty promise.

> > 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 totally agree about wanting to avoid reduced readability. In this case,
I actually think readability is improved since the macro "implementation"
are right above each #include. And then looking at the resulting included
header, all the metadata is visible in one place. But I agree: it is
"unusual", but I think on the sum it's an improvement. (But I share some
of the frustration of the kernel being filled with weird preprocessor
insanity. I will never get back the weeks I spent on trying to improve
the min/max macros.... *sob*)

> 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.

You mean 3/8's RUN_BPF_LSM_*_PROGS() additions to the call_*_hook()s?

I'll go comment on that thread directly instead of splitting the
discussion. :)

-- 
Kees Cook

Powered by blists - more mailing lists