Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx

[KP Singh]

Hi,

On Fri, Jun 19, 2020 at 2:49 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> On Wed, May 20, 2020 at 2:56 PM KP Singh <kpsingh@xxxxxxxxxxxx> wrote:
> > From: KP Singh <kpsingh@xxxxxxxxxx>
> >
> > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > hook by default, the call_int_hook logic is not suitable which
> > "bails-on-fail" and casues issues when other LSMs register this hook and
> > eventually breaks Audit.
> >
> > In order to fix this, directly iterate over the security hooks instead
> > of using call_int_hook as suggested in:
> >
> > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@xxxxxxxxxxxxxxxx/#t
> >
> > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > Reported-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> [...]
>
> Sorry for being late to the party, but doesn't this (and the
> associated default return value patch) just paper over a bigger
> problem? What if I have only the BPF LSM enabled and I attach a BPF
> program to this hook that just returns 0? Doesn't that allow anything
> privileged enough to do this to force the kernel to try and send
> memory from uninitialized pointers to userspace and/or copy such
> memory around and/or free uninitialized pointers?
>
> Why on earth does the BPF LSM directly expose *all* of the hooks, even
> those that are not being used for any security decisions (and are
> "useful" in this context only for borking the kernel...)? Feel free to
> prove me wrong, but this lazy approach of "let's just take all the
> hooks as they are and stick BPF programs to them" doesn't seem like a

The plan was definitely to not hook everywhere but only call the hooks
that do have a BPF program registered. This was one of the versions
we proposed in the initial patches where the call to the BPF LSM was
guarded by a static key with it being enabled only when there's a
BPF program attached to the hook.

https://lore.kernel.org/bpf/20200220175250.10795-5-kpsingh@xxxxxxxxxxxx/

However, this special-cased BPF in the LSM framework, and, was met
with opposition. Our plan is to still achieve this, but we want to do this
with DEFINE_STATIC_CALL patches:

https://lore.kernel.org/lkml/cover.1547073843.git.jpoimboe@xxxxxxxxxx

Using these, only can we enable the call into the hook based on whether
a program is attached, we can also eliminate the indirect call overhead which
currently affects the "slow" way which was decided in the discussion:

https://lore.kernel.org/bpf/202002241136.C4F9F7DFF@keescook/

> good choice... IMHO you should either limit the set of hooks that can
> be attached to only those that aren't used to return back values via

I am not sure if limiting the hooks is required here once we have
the ability to call into BPF only when a program is attached. If the
the user provides a BPF program, deliberately returns 0 (or any
other value) then it is working as intended. Even if we limit this in the
bpf LSM, deliberate privileged users can still achieve this with
other means.

- KP

> pointers, or (if you really really need to do some state
> updates/logging in those hooks) use wrapper functions that will call
> the BPF progs via a simplified interface so that they cannot cause
> unsafe behavior.
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>