[PATCH bpf-next v10 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached

[Paul Moore]

On Tue, May 7, 2024 at 8:01 PM Kees Cook <keescook at chromium.org> wrote:
>
> On Wed, May 08, 2024 at 12:10:45AM +0200, KP Singh wrote:
> > [...]
> > +/**
> > + * security_toggle_hook - Toggle the state of the LSM hook.
> > + * @hook_addr: The address of the hook to be toggled.
> > + * @state: Whether to enable for disable the hook.
> > + *
> > + * Returns 0 on success, -EINVAL if the address is not found.
> > + */
> > +int security_toggle_hook(void *hook_addr, bool state)
> > +{
> > +     struct lsm_static_call *scalls = ((void *)&static_calls_table);
> > +     unsigned long num_entries =
> > +             (sizeof(static_calls_table) / sizeof(struct lsm_static_call));
> > +     int i;
> > +
> > +     for (i = 0; i < num_entries; i++) {
> > +             if (!scalls[i].hl)
> > +                     continue;
> > +
> > +             if (scalls[i].hl->hook.lsm_func_addr != hook_addr)
> > +                     continue;
> > +
> > +             if (state)
> > +                     static_branch_enable(scalls[i].active);
> > +             else
> > +                     static_branch_disable(scalls[i].active);
> > +             return 0;
> > +     }
> > +     return -EINVAL;
> > +}
>
> First of all: patches 1-4 are great. They have a measurable performance
> benefit; let's get those in.
>
> But here I come to patch 5 where I will suggest the exact opposite of
> what Paul said in v9 for patch 5. :P

For those looking up v9 of the patchset, you'll be looking for patch
*4*, not patch 5, as there were only four patches in the v9 series.
Patch 4/5 in the v10 series is a new addition to the stack.

Beyond that, I'm guessing you are referring to my comment regarding
bpf_lsm_toggle_hook() Kees?  The one that starts with "More ugh.  If
we are going to solve things this way ..."?

> I don't want to have a global function that can be used to disable LSMs.
> We got an entire distro (RedHat) to change their SELinux configurations
> to get rid of CONFIG_SECURITY_SELINUX_DISABLE (and therefore
> CONFIG_SECURITY_WRITABLE_HOOKS), via commit f22f9aaf6c3d ("selinux:
> remove the runtime disable functionality"). We cannot reintroduce that,
> and I'm hoping Paul will agree, given this reminder of LSM history. :)
>
> Run-time hook changing should be BPF_LSM specific, if it exists at all.

I don't want individual LSMs manipulating the LSM hook state directly;
they go through the LSM layer to register their hooks, they should go
through the LSM layer to unregister or enable/disable their hooks.
I'm going to be pretty inflexible on this point.

Honestly, I see this more as a problem in the BPF LSM design (although
one might argue it's an implementation issue?), just as I saw the
SELinux runtime disable as a problem.  If you're upset with the
runtime hook disable, and you should be, fix the BPF LSM, don't force
more bad architecture on the LSM layer.

-- 
paul-moore.com