Safe LSM (un)loading, and immutable hooks

I think we can introduce a struct for passing arguments of security_add_hooks()/security_delete_hooks(). Then, we don't need to increment module usage count as many as hooks used.

---
 include/linux/lsm_hooks.h                     |  37 +++++---
 scripts/gcc-plugins/randomize_layout_plugin.c |   2 -
 security/apparmor/lsm.c                       |   6 +-
 security/commoncap.c                          |   7 +-
 security/loadpin/loadpin.c                    |   5 +-
 security/security.c                           | 117 ++++++++++++++------------
 security/selinux/hooks.c                      |  10 +--
 security/smack/smack_lsm.c                    |   4 +-
 security/tomoyo/tomoyo.c                      |   5 +-
 security/yama/yama_lsm.c                      |   5 +-
 10 files changed, 112 insertions(+), 86 deletions(-)

include/linux/lsm_hooks.h +14 -10

index c577d3d..4df62dd 100644
@@ -2007,10 +2007,17 @@ struct security_hook_heads {
  */
 struct security_hook_list {
        struct hlist_node                       list;
-       const unsigned int                      hook_idx;
+       const unsigned int                      offset;
        const union security_list_options       hook;
-       struct module                           *owner;
-       char                                    *lsm;
+       const char                              *lsm; /* Currently unused. */
+} __randomize_layout;
+
+struct lsm_info {
+       struct list_head                list;
+       const char                      *name;
+       struct module                   *owner;
+       const int                       count;
+       struct security_hook_list       *hooks;
 } __randomize_layout;
 
 /*

@@ -2020,20 +2027,27 @@ struct security_hook_list {
  * text involved.
  */
 #define HOOK_OFFSET(HEAD)      offsetof(struct security_hook_heads, HEAD)
-#define HOOK_SIZE(HEAD)                FIELD_SIZEOF(struct security_hook_heads, HEAD)
-#define LSM_HOOK_IDX(HEAD)     (HOOK_OFFSET(HEAD) / HOOK_SIZE(HEAD))
 
 #define LSM_HOOK_INIT(HEAD, HOOK) \
        {                                               \
                .hook = { .HEAD = HOOK },               \
-               .owner = THIS_MODULE,                   \
-               .hook_idx = LSM_HOOK_IDX(HEAD),         \
+               .offset = HOOK_OFFSET(HEAD),            \
        }
 extern char *lsm_names;
 
-extern int __must_check security_add_hooks(struct security_hook_list *hooks,
-                                               const int count, char *lsm,
-                                               const bool is_mutable);
+#define LSM_MODULE_INIT(NAME, HOOKS)           \
+       {                                       \
+               .name = NAME,                   \

include/linux/lsm_hooks.h +1 -2

 #ifdef CONFIG_SECURITY_UNREGISTRABLE_HOOKS
 /*
  * Used to facilitate runtime hook unloading

@@ -2049,8 +2063,7 @@ extern int __must_check security_add_hooks(struct security_hook_list *hooks,
  * disabling their module is a good idea needs to be at least as
  * careful as the SELinux team.
  */
-extern int __must_check security_delete_hooks(struct security_hook_list *hooks,
-                                               int count);
+extern int __must_check security_delete_hooks(struct lsm_info *lsm);
 #endif /* CONFIG_SECURITY_UNREGISTRABLE_HOOKS */
 
 extern int __init security_module_enable(const char *module);

scripts/gcc-plugins/randomize_layout_plugin.c +0 -2

index 6d5bbd3..d941389 100644
@@ -52,8 +52,6 @@ struct whitelist_entry {
        { "net/unix/af_unix.c", "unix_skb_parms", "char" },
        /* big_key payload.data struct splashing */
        { "security/keys/big_key.c", "path", "void *" },
-       /* walk struct security_hook_heads as an array of struct hlist_head */
-       { "security/security.c", "hlist_head", "security_hook_heads" },
        { }
 };
 

security/apparmor/lsm.c +4 -2

index 2a591bd..12c98aa8 100644
@@ -1191,6 +1191,9 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
        LSM_HOOK_INIT(task_kill, apparmor_task_kill),
 };
 
+static struct lsm_info apparmor_module =
+       LSM_MODULE_INIT("apparmor", apparmor_hooks);
+
 /*
  * AppArmor sysfs module parameters
  */

@@ -1562,8 +1565,7 @@ static int __init apparmor_init(void)
                aa_free_root_ns();
                goto buffers_out;
        }
-       BUG_ON(security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
-                                       "apparmor", false));
+       BUG_ON(security_add_hooks(&apparmor_module, false));
 
        /* Report that AppArmor successfully initialized */
        apparmor_initialized = 1;

security/commoncap.c +4 -3

index a215059..8c0df17 100644
@@ -1362,11 +1362,12 @@ struct security_hook_list capability_hooks[] __ro_after_init = {
        LSM_HOOK_INIT(vm_enough_memory, cap_vm_enough_memory),
 };
 
+static struct lsm_info capability_module =
+       LSM_MODULE_INIT("capability", capability_hooks);
+
 void __init capability_add_hooks(void)
 {
-       BUG_ON(security_add_hooks(capability_hooks,
-                                       ARRAY_SIZE(capability_hooks),
-                                       "capability", false));
+       BUG_ON(security_add_hooks(&capability_module, false));
 }
 
 #endif /* CONFIG_SECURITY */

security/loadpin/loadpin.c +4 -1

index 7c84ca8..60f85a5 100644
@@ -178,10 +178,13 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
        LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
 };
 
+static struct lsm_info loadpin_module =
+       LSM_MODULE_INIT("loadpin", loadpin_hooks);
+
 void __init loadpin_add_hooks(void)
 {
        pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
-       security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
+       BUG_ON(security_add_hooks(&loadpin_module, false));
 }
 
 /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */

security/security.c +7 -13

index dd69889..c4e5437 100644
@@ -32,9 +32,6 @@
 #include <linux/srcu.h>
 #include <linux/mutex.h>
 
-#define SECURITY_HOOK_COUNT \
-       (sizeof(security_hook_heads) / sizeof(struct hlist_head))
-
 #include <trace/events/initcall.h>
 
 #define MAX_LSM_EVM_XATTR      2

@@ -43,7 +40,7 @@
 #define SECURITY_NAME_MAX      10
 
 static struct security_hook_heads security_hook_heads __ro_after_init;
-
+static LIST_HEAD(registered_lsm_modules);
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 static DEFINE_MUTEX(security_hook_mutex);
 /*

@@ -93,20 +90,20 @@ static void __init do_security_initcalls(void)
 #define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) \
        hlist_for_each_entry(ITERATOR, &security_hook_heads_mutable.HEAD, list)
 
-static struct hlist_head *get_heads(bool is_mutable)
+static struct security_hook_heads *get_heads(bool is_mutable)
 {
        if (is_mutable)
-               return (struct hlist_head *)&security_hook_heads_mutable;
-       return (struct hlist_head *)&security_hook_heads;
+               return &security_hook_heads_mutable;
+       return &security_hook_heads;
 }
 #else
 #define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) while (0)
 
-static struct hlist_head *get_heads(bool is_mutable)
+static struct security_hook_heads *get_heads(bool is_mutable)
 {
        if (is_mutable)
                return ERR_PTR(-EINVAL);
-       return (struct hlist_head *)&security_hook_heads;
+       return &security_hook_heads;
 }
 #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
 

@@ -129,8 +126,7 @@ static inline void synchronize_lsm(void)
 
 /**
  * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
- * @hooks: the hooks to remove
- * @count: the number of hooks to remove
+ * @lsm: Module info,
  *
  * 0 is returned on success, otherwise -errno is returned on failure.
  * If an error is returned, it is up to the LSM author to handle the error

@@ -142,31 +138,29 @@ static inline void synchronize_lsm(void)
  * authors check the return code here, and act appropriately. In most cases
  * a failure should result in panic.
  */
-int __must_check security_delete_hooks(struct security_hook_list *hooks,
-                                       int count)

security/security.c +13 -19

+       mutex_unlock(&security_hook_mutex);
        return ret;
 }
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
 static void lock_existing_hooks(void)
 {
-       struct hlist_head *list = (struct hlist_head *)
-                                       &security_hook_heads_mutable;
-       struct security_hook_list *P;
-       int i;
+       struct lsm_info *lsm;
 
        /*
         * Prevent module unloading while we're doing this

@@ -174,10 +168,8 @@ static void lock_existing_hooks(void)
         * is already unloading -- allow that.
         */
        mutex_lock(&module_mutex);
-       for (i = 0; i < SECURITY_HOOK_COUNT; i++)
-               hlist_for_each_entry(P, &list[i], list)
-                       if (P->owner)
-                               WARN_ON(!try_module_get(P->owner));
+       list_foreach_entry(lsm, &registered_lsm_modules, list)
+               try_module_get(lsm->owner);
        mutex_unlock(&module_mutex);
 }
 #else

@@ -198,16 +190,19 @@ static inline void synchronize_lsm(void) {}
  * Only to be called by legacy code, like SELinux's delete hooks mechanism
  * as it ignores whether or not allow_unregister_hooks is set.
  */
-void __security_delete_hooks(struct security_hook_list *hooks, int count)
+void __security_delete_hooks(struct lsm_info *lsm)
 {
        int i;
+       struct security_hook_list *hooks = lsm->hooks;
+       const int count = lsm->count;
 
        mutex_lock(&security_hook_mutex);
        for (i = 0; i < count; i++)
                hlist_del_rcu(&hooks[i].list);
+       list_del(&lsm->list);
+       synchronize_lsm();
        mutex_unlock(&security_hook_mutex);
 
-       synchronize_lsm();
 }
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 

@@ -314,7 +309,7 @@ static bool match_last_lsm(const char *list, const char *lsm)
        return !strcmp(last, lsm);
 }
 
-static int lsm_append(char *new, char **result)
+static int lsm_append(const char *new, char **result)
 {
        char *cp;
 

@@ -358,41 +353,53 @@ int __init security_module_enable(const char *module)
 
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
- * @hooks: the hooks to add
- * @count: the number of hooks to add
- * @lsm: the name of the security module
+ * @lsm: Module info,
  * @is_mutable: True if dynamic registration and/or unregistration is needed.
  *
  * Each LSM has to register its hooks with the infrastructure.
  * 0 is returned on success, otherwise -errno is returned on failure.
  */
-int __must_check security_add_hooks(struct security_hook_list *hooks,
-                                       const int count, char *lsm,
-                                       const bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *lsm,
+                                   const bool is_mutable)
 {
-       struct hlist_head *heads;
-       int i, hook_idx, ret = 0;

security/security.c +14 -11

+       if (mutex_lock_killable(&security_hook_mutex)) {
+               module_put(lsm->owner);
+               return -EINTR;
+       }
+       ret = lsm_append(lsm->name, &lsm_names);
+       if (ret) {
+               module_put(lsm->owner);
                goto out;
        }
-
+       list_add(&lsm->list, &registered_lsm_modules);
        for (i = 0; i < count; i++) {
-               hook_idx = hooks[i].hook_idx;
-               hooks[i].lsm = lsm;
-               hlist_add_tail_rcu(&hooks[i].list, &heads[hook_idx]);
-               if (!security_allow_unregister_hooks && hooks[i].owner)
-                       WARN_ON(!try_module_get(hooks->owner));
-       }
+               struct hlist_head *head = (struct hlist_head *)
+                       (((char *) heads) + hooks[i].offset);
 
-       if (lsm_append(lsm, &lsm_names) < 0)
-               panic("%s - Cannot get memory.\n", __func__);
-out:
+               hooks[i].lsm = lsm->name;
+               hlist_add_tail_rcu(&hooks[i].list, head);
+       }
+ out:
        mutex_unlock(&security_hook_mutex);
-
        return ret;
 }
 EXPORT_SYMBOL_GPL(security_add_hooks);

security/selinux/hooks.c +5 -5

index 72466eb..0140e2b 100644
@@ -7090,6 +7090,9 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
 #endif
 };
 
+static struct lsm_info selinux_module =
+       LSM_MODULE_INIT("selinux", selinux_hooks);
+
 static __init int selinux_init(void)
 {
        const bool is_mutable = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE);

@@ -7131,8 +7134,7 @@ static __init int selinux_init(void)
 
        hashtab_cache_init();
 
-       BUG_ON(security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
-                                       "selinux", is_mutable));
+       BUG_ON(security_add_hooks(&selinux_module, is_mutable));
 
        if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
                panic("SELinux: Unable to register AVC netcache callback\n");

@@ -7266,8 +7268,6 @@ static void selinux_nf_ip_exit(void)
  * it not meant to be  used by new LSMs. Therefore, this is defined here, rather
  * than in a shared header.
  */
-extern void __security_delete_hooks(struct security_hook_list *hooks,
-                                       int count);
 int selinux_disable(struct selinux_state *state)
 {
        if (state->initialized) {

@@ -7286,7 +7286,7 @@ int selinux_disable(struct selinux_state *state)
 
        selinux_enabled = 0;
 
-       __security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+       __security_delete_hooks(&selinux_module);
 
        /* Try to destroy the avc node cache */
        avc_disable();

security/smack/smack_lsm.c +2 -2

index f69b4d9..6b9566e 100644
@@ -4764,6 +4764,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
        LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
 };
 
+static struct lsm_info smack_module = LSM_MODULE_INIT("smack", smack_hooks);
 
 static __init void init_smack_known_list(void)
 {

@@ -4842,8 +4843,7 @@ static __init int smack_init(void)
        /*
         * Register with LSM
         */
-       BUG_ON(security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack",
-                                       false));
+       BUG_ON(security_add_hooks(&smack_module, false));
 
        return 0;
 }

security/tomoyo/tomoyo.c +3 -2

index 544a614..0917f71 100644
@@ -528,6 +528,8 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
        LSM_HOOK_INIT(socket_sendmsg, tomoyo_socket_sendmsg),
 };
 
+static struct lsm_info tomoyo_module = LSM_MODULE_INIT("tomoyo", tomoyo_hooks);
+
 /* Lock for GC. */
 DEFINE_SRCU(tomoyo_ss);
 

@@ -543,8 +545,7 @@ static int __init tomoyo_init(void)
        if (!security_module_enable("tomoyo"))
                return 0;
        /* register ourselves with the security framework */
-       BUG_ON(security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo",
-                                       false));
+       BUG_ON(security_add_hooks(&tomoyo_module, false));
        printk(KERN_INFO "TOMOYO Linux initialized\n");
        cred->security = &tomoyo_kernel_domain;
        tomoyo_mm_init();

security/yama/yama_lsm.c +3 -3

index 2e4bbb0..ee520b0 100644
@@ -430,6 +430,8 @@ int yama_ptrace_traceme(struct task_struct *parent)
        LSM_HOOK_INIT(task_free, yama_task_free),
 };
 
+static struct lsm_info yama_module = LSM_MODULE_INIT("yama", yama_hooks);
+
 #ifdef CONFIG_SYSCTL
 static int yama_dointvec_minmax(struct ctl_table *table, int write,
                                void __user *buffer, size_t *lenp, loff_t *ppos)

@@ -480,7 +482,6 @@ static inline void yama_init_sysctl(void) { }
 void __init yama_add_hooks(void)
 {
        pr_info("Yama: becoming mindful.\n");
-       BUG_ON(security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama",
-                                       false));
+       BUG_ON(security_add_hooks(&yama_module, false));
        yama_init_sysctl();
 }
--

1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

include/linux/lsm_hooks.h struct security_hook_list { · line 2050

Quoted 13 lines · Tetsuo Handa

include/linux/lsm_hooks.h +13 -0

+#define LSM_MODULE_INIT(NAME, HOOKS)           \
+       {                                       \
+               .name = NAME,                   \
+               .hooks = HOOKS,                 \
+               .count = ARRAY_SIZE(HOOKS),     \
+               .owner = THIS_MODULE,           \
+       }
+
+extern int __must_check security_add_hooks(struct lsm_info *lsm,
+                                          const bool is_mutable);
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+extern void __security_delete_hooks(struct lsm_info *lsm);
+#endif

Why expose __security_delete_hooks in lsm_hooks.h? Given SELinux is the only consumer, and shouldn't be using it much longer, I think it makes sense to let it stay part of the SELinux hooks.c.

Quoted 10 lines

security/security.c +6 -2

+int __must_check security_add_hooks(struct lsm_info *lsm,
+                                   const bool is_mutable)
 {
-       struct hlist_head *heads;
-       int i, hook_idx, ret = 0;
+       struct security_hook_heads *heads = get_heads(is_mutable);
+       struct security_hook_list *hooks = lsm->hooks;
+       const int count = lsm->count;
+       int i, ret;

security/security.c int __init security_module_enable(const char *module) · line 370

Quoted 5 lines

security/security.c +1 -4

-       mutex_lock(&security_hook_mutex);
-       heads = get_heads(is_mutable);
-       if (IS_ERR(heads)) {
-               ret = PTR_ERR(heads);
+       INIT_LIST_HEAD(&lsm->list);

Can't this happen in the macro to setup LSM_INFO?

security/security.c int __init security_module_enable(const char *module) · line 383

Quoted 13 lines

security/security.c +13 -0

+       if (IS_ERR(heads))
+               return PTR_ERR(heads);
+       for (i = 0; i < count; i++) {
+               unsigned int offset = hooks[i].offset;
+
+               if (offset % sizeof(struct hlist_head) ||
+                   offset + sizeof(struct hlist_head) >
+                   sizeof(struct security_hook_heads))
+                       return -EINVAL;
+       }
+       if (!security_allow_unregister_hooks &&
+           !try_module_get(lsm->owner))
+               return -EINVAL;

What case is it okay if try_module_get fails? Won't this return -EINVAL on built-in modules because lsm->owner is NULL?

security/security.c static inline void synchronize_lsm(void) · line 147

Quoted 10 lines

security/security.c +5 -3

+int __must_check security_delete_hooks(struct lsm_info *lsm)
 {
-       int i, ret = 0;
+       struct security_hook_list *hooks = lsm->hooks;
+       const int count = lsm->count;
+       int i, ret = -EPERM;
 
-       mutex_lock(&security_hook_mutex);
-       if (security_allow_unregister_hooks)
+       if (mutex_lock_killable(&security_hook_mutex))

Why mutex_lock_killable?