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

KP Singh kpsingh at kernel.org
Wed May 8 07:00:42 UTC 2024



> On 8 May 2024, at 03:45, Paul Moore <paul at paul-moore.com> wrote:
> 
> 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.


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 xat>
        {                                               \
                .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),
+ * 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),
kpsingh at kpsingh:~/projects/linux$ git diff
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;

- KP

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





More information about the Linux-security-module-archive mailing list