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

[Kees Cook <keescook@chromium.org>]

On Wed, May 08, 2024 at 09:00:42AM +0200, KP Singh wrote:
> 
> 
> > On 8 May 2024, at 03:45, Paul Moore <paul@paul-moore.com> wrote:
> > 
> > On Tue, May 7, 2024 at 8:01 PM Kees Cook <keescook@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.
> 
> 
> One idea here is that only LSM hooks with default_state = false can be toggled. 
> 
> This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic)
> 
> and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground?
> 
> Something like:
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4bd1d47bb9dc..5c0918ed6b80 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -117,7 +117,7 @@ struct security_hook_list {
>         struct lsm_static_call  *scalls;
>         union security_list_options     hook;
>         const struct lsm_id             *lsmid;
> -       bool                            default_enabled;
> +       bool                            toggleable;
>  } __randomize_layout;
> 
>  /*
> @@ -168,14 +168,18 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
>         {                                               \
>                 .scalls = static_calls_table.NAME,      \
>                 .hook = { .NAME = HOOK },               \
> -               .default_enabled = true                 \
> +               .toggleable = false                     \
>         }
> 
> -#define LSM_HOOK_INIT_DISABLED(NAME, HOOK)             \
> +/*
> + * Toggleable LSM hooks are enabled at runtime with
> + * security_toggle_hook and are initialized as inactive.
> + */
> +#define LSM_HOOK_INIT_TOGGLEABLE(NAME, HOOK)           \
>         {                                               \
>                 .scalls = static_calls_table.NAME,      \
>                 .hook = { .NAME = HOOK },               \
> -               .default_enabled = false                \
> +               .toggleable = true                      \
>         }
> 
>  extern char *lsm_names;
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> index ed864f7430a3..ba1c3a19fb12 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -9,7 +9,7 @@
> 
>  static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = {
>         #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> -       LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME),
> +       LSM_HOOK_INIT_TOGGLEABLE(NAME, bpf_lsm_##NAME),
>         #include <linux/lsm_hook_defs.h>
>         #undef LSM_HOOK
>         LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
> diff --git a/security/security.c b/security/security.c
> index b3a92a67f325..a89eb8fe302b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -407,7 +407,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl)
>                         __static_call_update(scall->key, scall->trampoline,
>                                              hl->hook.lsm_func_addr);
>                         scall->hl = hl;
> -                       if (hl->default_enabled)
> +                       /* Toggleable hooks are inactive by default */
> +                       if (!hl->toggleable)
>                                 static_branch_enable(scall->active);
>                         return;
>                 }
> @@ -901,6 +902,9 @@ int security_toggle_hook(void *hook_addr, bool state)
>         int i;
> 
>         for (i = 0; i < num_entries; i++) {
> +               if (!scalls[i].hl->toggleable)
> +                       continue;
> +
>                 if (!scalls[i].hl)
>                         continue;

Yeah, I like this! It's a routine that is walking read-only data to make
the choice, and it's specific to a pre-defined characteristic that an
LSM would need to opt into. My concerns are addressed! Thanks! :)

-- 
Kees Cook