Safe LSM (un)loading, and immutable hooks

Previously, when LSMs registered, they independently passed their name and hook count. This had two implications:

1) Is required us to clone the name, so we could present it in security FS. This required memory allocation at start time.
2) Every time we wanted to tie more information back from the security hooks, to the LSM, we would have to add duplicated fields in struct security_hook_list.

It also introduces a new file -- security/security.h, which is meant to be private headers to be shared only between pieces of security "infrastructure".

Signed-off-by Sargun Dhillon <[email protected]>

---
 include/linux/lsm_hooks.h  | 44 ++++++++++-------------
 security/apparmor/lsm.c    |  6 ++--
 security/commoncap.c       |  8 +++--
 security/inode.c           | 56 +++++++++++++++++++++++++----
 security/loadpin/loadpin.c |  4 ++-
 security/security.c        | 89 +++++++++++++++++++++++-----------------------
 security/security.h        | 10 ++++++
 security/selinux/hooks.c   |  6 ++--
 security/smack/smack_lsm.c |  3 +-
 security/tomoyo/tomoyo.c   |  4 ++-
 security/yama/yama_lsm.c   |  4 ++-
 11 files changed, 147 insertions(+), 87 deletions(-)
 create mode 100644 security/security.h

include/linux/lsm_hooks.h +5 -0

index 9d0b286f3dba..65f346cb6639 100644
@@ -2004,11 +2004,20 @@ struct security_hook_heads {
  * Security module hook list structure.
  * For use with generic list macros for common operations.
  */
+struct security_hook_list;
+struct lsm_info {
+       struct hlist_node               list;
+       const char                      *name;
+       const unsigned int              count;

include/linux/lsm_hooks.h +0 -0

 } __randomize_layout;
 
 /*

@@ -2020,33 +2029,18 @@ struct security_hook_list {
 #define LSM_HOOK_INIT(HEAD, HOOK) \

Quoted 42 lines · Sargun Dhillon

Previously, when LSMs registered, they independently passed their name and hook count. This had two implications:

1) Is required us to clone the name, so we could present it in security FS. This required memory allocation at start time.
2) Every time we wanted to tie more information back from the security hooks, to the LSM, we would have to add duplicated fields in struct security_hook_list.

It also introduces a new file -- security/security.h, which is meant to be private headers to be shared only between pieces of security "infrastructure".

Signed-off-by Sargun Dhillon <[email protected]>

---
 include/linux/lsm_hooks.h  | 44 ++++++++++-------------
 security/apparmor/lsm.c    |  6 ++--
 security/commoncap.c       |  8 +++--
 security/inode.c           | 56 +++++++++++++++++++++++++----
 security/loadpin/loadpin.c |  4 ++-
 security/security.c        | 89 +++++++++++++++++++++++-----------------------
 security/security.h        | 10 ++++++
 security/selinux/hooks.c   |  6 ++--
 security/smack/smack_lsm.c |  3 +-
 security/tomoyo/tomoyo.c   |  4 ++-
 security/yama/yama_lsm.c   |  4 ++-
 11 files changed, 147 insertions(+), 87 deletions(-)
 create mode 100644 security/security.h

include/linux/lsm_hooks.h +5 -0

index 9d0b286f3dba..65f346cb6639 100644
@@ -2004,11 +2004,20 @@ struct security_hook_heads {
  * Security module hook list structure.
  * For use with generic list macros for common operations.
  */
+struct security_hook_list;
+struct lsm_info {
+       struct hlist_node               list;
+       const char                      *name;
+       const unsigned int              count;

include/linux/lsm_hooks.h struct security_hook_heads { · line 2020

Quoted 10 lines

include/linux/lsm_hooks.h +5 -1

+       struct security_hook_list       *hooks;
+} __randomize_layout;
+
 struct security_hook_list {
        struct hlist_node               list;
        struct hlist_head               *head;
        union security_list_options     hook;
-       char                            *lsm;
+       /* This field is not currently in use */
+       struct lsm_info                 *info;

const?

Quoted 5 lines

include/linux/lsm_hooks.h +0 -0

 } __randomize_layout;
 
 /*

@@ -2020,33 +2029,18 @@ struct security_hook_list {
 #define LSM_HOOK_INIT(HEAD, HOOK) \

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

Quoted 10 lines

include/linux/lsm_hooks.h +6 -2

        { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
 
-extern struct security_hook_heads security_hook_heads;
-extern char *lsm_names;
+#define LSM_MODULE_INIT(NAME, HOOKS)           \
+       {                                       \
+               .name   = NAME,                 \
+               .hooks  = HOOKS,                \
+               .count  = ARRAY_SIZE(HOOKS),    \
+       }

Instead of leaving this so every LSM has to do all the declarations, how about:

#define LSM_MODULE(NAME) \
    static const struct lsm_info NAME ## _info = { \
        .name = #NAME, \
        .hooks = NAME ## _hooks, \
        .count = ARRAY_SIZE(NAME ## _hooks), \
    }

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

Quoted 2 lines

include/linux/lsm_hooks.h +2 -0

+static struct lsm_info apparmor_info =
+       LSM_MODULE_INIT("apparmor", apparmor_hooks);

This becomes just:

LSM_MODULE(apparmor);

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

Quoted 2 lines

include/linux/lsm_hooks.h +2 -0

+static struct lsm_info capability_info =
+       LSM_MODULE_INIT("capability", capability_hooks);

LSM_MODULE(capability);

etc...

-Kees

--
Kees Cook
Pixel Security

Suggested changes on top of your series.

But I think that your series still lacks critical code which is also not yet implemented in Casey's work. The reason call_int_hook() can work for hooks which change state (e.g. security_inode_alloc()) is that there is no need to call undo function because a hook which might change state (so-called Major LSM modules) is always the last entry of the list. If we allow runtime registration of LSM modules, there is a possibility that call_int_hook() returns an error after some LSM module allocated memory. You need to implement safe transaction in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.

---
 include/linux/lsm_hooks.h |  4 +--
 security/security.c       | 86 +++++++++++++----------------------------------
 security/security.h       |  5 ---
 3 files changed, 25 insertions(+), 70 deletions(-)

include/linux/lsm_hooks.h +2 -2

index 5c227bb..98809d6 100644
@@ -2010,7 +2010,7 @@ struct lsm_info {
        const char                      *name;
        const unsigned int              count;
        struct security_hook_list       *hooks;
-       struct module                   *owner;
+       const struct module             *owner;
 } __randomize_layout;
 
 struct security_hook_list {

@@ -2044,7 +2044,7 @@ struct security_hook_list {
        }
 
 extern int __must_check security_add_hooks(struct lsm_info *lsm,
-                                               bool is_mutable);
+                                          bool is_writable);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 void __security_delete_hooks(struct lsm_info *lsm);

security/security.c +1 -1

index 3606512..a4bc9cd 100644
@@ -48,18 +48,15 @@
 
 /*
  * security_allow_unregister_hooks blocks the delete_module syscall for
- * hooks that are loaded into the  set of mutable hooks by getting a reference
+ * hooks that are loaded into the set of writable hooks by getting a reference
  * on those modules. This allows built-in modules to still delete their security
  * hooks, so SELinux will still be able to deregister.
  *
  * If an arbitrary module tries to deregister, it will get a return code

security/security.c +5 -5

 #define FOR_EACH_SECURITY_HOOK_RO(ITERATOR, HEAD) \
        hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_ro.HEAD, list)

@@ -90,18 +87,18 @@ void __security_delete_hooks(struct lsm_info *info)
 #define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD) \
        hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_rw.HEAD, list)
 
-struct security_hook_heads *get_security_hook_heads(bool is_mutable)
+static struct security_hook_heads *get_security_hook_heads(bool is_writable)
 {
-       if (is_mutable)
+       if (is_writable)
                return &security_hook_heads_rw;
        return &security_hook_heads_ro;
 }
 #else
 #define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD)      while (0)
 
-struct security_hook_heads *get_security_hook_heads(bool is_mutable)
+static struct security_hook_heads *get_security_hook_heads(bool is_writable)
 {
-       if (is_mutable)
+       if (is_writable)
                return ERR_PTR(-ENOTSUPP);
        return &security_hook_heads_ro;
 }

@@ -120,7 +117,7 @@ static inline void unlock_lsm(int idx)
 }
 
 /**
- * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
+ * security_delete_hooks - Remove modules hooks from the writable hooks lists.
  * @lsm_info: The lsm_info struct for this security module
  *
  * If LSM unloading is disabled, this function will panic. It should not

security/security.c +0 -9

        __security_delete_hooks(info);
 

@@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
 }
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
-static void lock_existing_hooks(void)
-{
-       struct lsm_info *info;
-
-       /*
-        * Prevent module unloading while we're doing this
-        * try_module_get may fail (safely), if the module
-        * is already unloading -- allow that.
-        */

security/security.c +4 -5

        /* Noop */
-       if (!!new_val == !!current_val)
+       if (new_val == current_val)
                return 0;
 
        /* Do not allow for the transition from false -> true */

@@ -178,16 +152,15 @@ static int allow_unregister_hooks_set(const char *val,
                return -EPERM;
 
        /* The only legal transition true -> false */
-       if (atomic_cmpxchg(&security_allow_unregister_hooks, 1, 0) == 1)
-               lock_existing_hooks();
-
+       if (!xchg(&security_allow_unregister_hooks, 1))
+               pr_info("Locked security hooks.\n");
        return 0;
 }
 
 static int allow_unregister_hooks_get(char *buffer,
                                        const struct kernel_param *kp)
 {
-       const int current_val = atomic_read(&security_allow_unregister_hooks);
+       const bool current_val = security_allow_unregister_hooks;
        int ret;
 
        ret = sprintf(buffer, "%c\n", current_val ? '1' : '0');

@@ -241,14 +214,13 @@ static int security_module_cb(struct notifier_block *nb, unsigned long val,
        struct module *mod = data;
        struct lsm_info *info;
 

security/security.c +5 -17

        mutex_unlock(&lsm_info_lock);
 
        return NOTIFY_DONE;

@@ -334,41 +306,29 @@ static void security_add_hook(struct security_hook_list *hook,
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @lsm_info: The lsm_info struct for this security module
- * @is_mutable: Can these hooks be loaded or unloaded after boot time
+ * @is_writable: Can these hooks be loaded or unloaded after boot time
  *
  * Each LSM has to register its hooks with the infrastructure.
  * Return 0 on success
  */
-int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *info, bool is_writable)
 {
        struct security_hook_heads *heads;
-       int i, ret = 0;
+       int i;
 
-       heads = get_security_hook_heads(is_mutable);
+       heads = get_security_hook_heads(is_writable);
        if (IS_ERR(heads))
                return PTR_ERR(heads);
 
        if (mutex_lock_killable(&lsm_info_lock))
                return -EINTR;
 
-       if (!atomic_read(&security_allow_unregister_hooks)) {
-               /*
-                * Because hook deregistration is not allowed, we need to try
-                * to get a refcount on the module owner.
-                */
-               if (!try_module_get(info->owner)) {
-                       ret = -EINVAL;
-                       goto out;
-               }
-       }
-
        for (i = 0; i < info->count; i++)
                security_add_hook(&info->hooks[i], info, heads);
        hlist_add_tail_rcu(&info->list, &lsm_info_head);
-out:
        mutex_unlock(&lsm_info_lock);
 
-       return ret;
+       return 0;
 }
 EXPORT_SYMBOL_GPL(security_add_hooks);
 

security/security.h +0 -0

index 3e757f5..ed056d5 100644

security/security.h +0 -3

 extern struct mutex lsm_info_lock;
 extern struct hlist_head lsm_info_head;
-extern struct security_hook_heads *get_security_hook_heads(bool is_mutable);
-#endif
--

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

Quoted 11 lines · Tetsuo Handa

Suggested changes on top of your series.

But I think that your series still lacks critical code which is also not yet implemented in Casey's work. The reason call_int_hook() can work for hooks which change state (e.g. security_inode_alloc()) is that there is no need to call undo function because a hook which might change state (so-called Major LSM modules) is always the last entry of the list. If we allow runtime registration of LSM modules, there is a possibility that call_int_hook() returns an error after some LSM module allocated memory. You need to implement safe transaction in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.

My suggestion is we create a whitelist of "minor" hooks". These hooks should have no state, or blob manipulation. The only downside here, is sometimes you can cheat, and do what Yama does, in the sense of keeping its own relationships completely outside of the blobs which live on the actually tasks structs themselves. Given that Yama has done this successfully, I think we should trust module authors to not make these mistakes.

Quoted 22 lines

security/security.c +0 -18

@@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
 }
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
-static void lock_existing_hooks(void)
-{
-       struct lsm_info *info;
-
-       /*
-        * Prevent module unloading while we're doing this
-        * try_module_get may fail (safely), if the module
-        * is already unloading -- allow that.
-        */
-       mutex_lock(&module_mutex);
-       mutex_lock(&lsm_info_lock);
-       pr_info("Locking security hooks\n");
-
-       hlist_for_each_entry(info, &lsm_info_head, list)
-               WARN_ON(!try_module_get(info->owner));
-
-       mutex_unlock(&lsm_info_lock);
-       mutex_unlock(&module_mutex);

Review void security_delete_hooks(struct lsm_info *info) · line 159

Quoted 1 line

(quoted diff) +0 -1

-}

Why do you think we should remove this code? I think that it's valuable to keep in there because it prevents an administrator from accidentally panicing the system. For example, if you have admin A lock the security hooks, and then admin B comes along and tries to unload the module, IMHO they shouldn't get a panic, and by default (unless they rmmod -f) they should just a nice warning that suggests they're doing something wrong (like the module is in use).

Thanks for the feedback. I think we're getting there at last.

I'd like Casey and James' feedback as well. I think the big questions are:
1) Can we assume that LSM authors will not shoot themselves in the foot and break major LSMs?

2) Should we protect the administrator from themselves (try_module_get)?

3) Is it okay to allow hooks to be unloaded at all?

I think no matter what the answers to 1-3 are, we can apply patch 1, 2, and 3 once I include the fix-ups that you suggested here.

Patch 4 is heavily dependent on the answer to (1), and patches 6 is heavily dependent on the answer to (3).

Quoted 13 lines · Sargun Dhillon

Quoted 11 lines · Tetsuo Handa

Suggested changes on top of your series.

But I think that your series still lacks critical code which is also not yet implemented in Casey's work. The reason call_int_hook() can work for hooks which change state (e.g. security_inode_alloc()) is that there is no need to call undo function because a hook which might change state (so-called Major LSM modules) is always the last entry of the list. If we allow runtime registration of LSM modules, there is a possibility that call_int_hook() returns an error after some LSM module allocated memory. You need to implement safe transaction in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.

Quoted 7 lines

My suggestion is we create a whitelist of "minor" hooks". These hooks should have no state, or blob manipulation. The only downside here, is sometimes you can cheat, and do what Yama does, in the sense of keeping its own relationships completely outside of the blobs which live on the actually tasks structs themselves. Given that Yama has done this successfully, I think we should trust module authors to not make these mistakes.

I didn't understand what you mean. Basically, what this series is doing is

int security_inode_alloc(struct inode *inode)

{
        struct security_hook_list *P;
        inode->i_security = NULL;
        hlist_for_each_entry(P, &security_hook_heads_ro.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0)
                        return RC; // <= So far only one major module can stay on the list.
        }

        /*** Start of changes made by this series ***/
        hlist_for_each_entry(P, &security_hook_heads_rw.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0)
                        return RC; // <= Not calling inode_free_security() for corresponding successful inode_alloc_security().

        }
        /*** End of changes made by this series ***/
        return 0;
}

and what Casey's series is doing is

int security_inode_alloc(struct inode *inode)

{
        struct security_hook_list *P;
        inode->i_security = NULL;
        hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0) {
                        /*** Start of changes made by Casey's series ***/
                        hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
                                P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
                        }
                        /*** End of changes made by Casey's series ***/
                        return RC;
                }
        }
        return 0;
}

. We need to keep track of which module's inode_free_security() needs to be called

int security_inode_alloc(struct inode *inode)

{
        struct security_hook_list *P1;
        struct security_hook_list *P2;
        inode->i_security = NULL;
        hlist_for_each_entry(P1, &security_hook_heads_ro.inode_alloc_security, list) {
                int RC = P1->hook.inode_alloc_security(inode);
                if (RC != 0)
                        goto out;
        }
        hlist_for_each_entry(P1, &security_hook_heads_rw.inode_alloc_security, list) {
                int RC = P1->hook.inode_alloc_security(inode);
                if (RC != 0)
                        goto out;
        }
        return 0;
out:
        hlist_for_each_entry(P2, &security_hook_heads_ro.inode_free_security, list) {
                if (P1 == P2)
                        goto done;
                P2->hook.inode_free_security(inode);
        }
        hlist_for_each_entry(P2, &security_hook_heads_rw.inode_free_security, list) {
                if (P1 == P2)
                        goto done;
                P2->hook.inode_free_security(inode);
        }
done:
        return ret;
}

while handling race condition that foo->inode_alloc_security(inode) returned an error and foo is removed from the list and is waiting for SRCU grace period before hitting P1 == P2 check and therefore by error calls all LSM modules' ->inode_free_security(inode) hooks.

Quoted 22 lines

Quoted 22 lines

(quoted diff) +0 -18

@@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
 }
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
-static void lock_existing_hooks(void)
-{
-       struct lsm_info *info;
-
-       /*
-        * Prevent module unloading while we're doing this
-        * try_module_get may fail (safely), if the module
-        * is already unloading -- allow that.
-        */
-       mutex_lock(&module_mutex);
-       mutex_lock(&lsm_info_lock);
-       pr_info("Locking security hooks\n");
-
-       hlist_for_each_entry(info, &lsm_info_head, list)
-               WARN_ON(!try_module_get(info->owner));
-
-       mutex_unlock(&lsm_info_lock);
-       mutex_unlock(&module_mutex);

Quoted 8 lines

Review void security_delete_hooks(struct lsm_info *info) · line 159

Quoted 1 line

(quoted diff) +0 -1

-}

Why do you think we should remove this code? I think that it's valuable to keep in there because it prevents an administrator from accidentally panicing the system. For example, if you have admin A lock the security hooks, and then admin B comes along and tries to unload the module, IMHO they shouldn't get a panic, and by default (unless they rmmod -f) they should just a nice warning that suggests they're doing something wrong (like the module is in use).

What I suggested is

        /* Doing init or already dying? */
        if (mod->state != MODULE_STATE_LIVE) {
                /* FIXME: if (force), slam module count damn the torpedoes */
                pr_debug("%s already dying\n", mod->name);
                ret = -EBUSY;
                goto out;
        }

(quoted diff) +3 -0

+       ret = security_remove_module(mod);
+       if (ret)
+               goto out;
        /* If it has an init func, it must have an exit func to unload */
        if (mod->init && !mod->exit) {
                forced = try_force_unload(flags);

and check like

int security_remove_module(struct module *mod)

{
        struct lsm_info *info;
        int ret = 0;

        if (security_allow_unregister_hooks)
                return 0;

        mutex_lock(&lsm_info_lock);
        hlist_for_each_entry(info, &lsm_info_head, list)
                if (mod == info->owner)
                        ret = -EPERM;
        mutex_unlock(&lsm_info_lock);
        return ret;
}

rather than using register_module_notifier().

Quoted 5 lines

Thanks for the feedback. I think we're getting there at last.

I'd like Casey and James' feedback as well. I think the big questions are:
1) Can we assume that LSM authors will not shoot themselves in the foot and break major LSMs?

What "break major LSMs" means? Regardless of whether LKM-based LSMs use inode->i_security, we need to handle race condition described above.

Quoted 1 line

2) Should we protect the administrator from themselves (try_module_get)?

What I suggested is "not to play with module usage count".

Quoted 5 lines

3) Is it okay to allow hooks to be unloaded at all?

I think no matter what the answers to 1-3 are, we can apply patch 1, 2, and 3 once I include the fix-ups that you suggested here.

Quoted 2 lines

Patch 4 is heavily dependent on the answer to (1), and patches 6 is heavily dependent on the answer to (3).

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

Quoted 25 lines · Tetsuo Handa

Quoted 13 lines · Sargun Dhillon

Quoted 11 lines · Tetsuo Handa

Suggested changes on top of your series.

But I think that your series still lacks critical code which is also not yet implemented in Casey's work. The reason call_int_hook() can work for hooks which change state (e.g. security_inode_alloc()) is that there is no need to call undo function because a hook which might change state (so-called Major LSM modules) is always the last entry of the list. If we allow runtime registration of LSM modules, there is a possibility that call_int_hook() returns an error after some LSM module allocated memory. You need to implement safe transaction in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.

Quoted 7 lines

My suggestion is we create a whitelist of "minor" hooks". These hooks should have no state, or blob manipulation. The only downside here, is sometimes you can cheat, and do what Yama does, in the sense of keeping its own relationships completely outside of the blobs which live on the actually tasks structs themselves. Given that Yama has done this successfully, I think we should trust module authors to not make these mistakes.

I didn't understand what you mean. Basically, what this series is doing is

int security_inode_alloc(struct inode *inode)

Quoted 8 lines

{
        struct security_hook_list *P;
        inode->i_security = NULL;
        hlist_for_each_entry(P, &security_hook_heads_ro.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0)
                        return RC; // <= So far only one major module can stay on the list.
        }

If no major LSMs exist in the RW series, it'll fall through to here.

Quoted 5 lines

        /*** Start of changes made by this series ***/
        hlist_for_each_entry(P, &security_hook_heads_rw.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0)
                        return RC; // <= Not calling inode_free_security() for corresponding successful inode_alloc_security().

inode_free_security is called on all LSM callbacks -- so, it should be, as long as people don't load unloadable major LSMs. If we want to be super conservative here, we can do a try_module_get, and a module_put if RC != 0 on RW hooks. So, we could have something like call_int_hook_major, and call_void_hook_major.

Quoted 8 lines

        }
        /*** End of changes made by this series ***/
        return 0;
}

and what Casey's series is doing is

int security_inode_alloc(struct inode *inode)

Quoted 16 lines

{
        struct security_hook_list *P;
        inode->i_security = NULL;
        hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0) {
                        /*** Start of changes made by Casey's series ***/
                        hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
                                P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
                        }
                        /*** End of changes made by Casey's series ***/
                        return RC;
                }
        }
        return 0;
}

Right, but this could be taken care of by just ensuring that nobody allocates blobs (major LSM), and only one LSM returns a non-zero to the *alloc* callbacks? Right now, we have this because one has to be a "major" LSM in order to use these callbacks, and we ensure only one major LSM is active at a time.

I suggest that either in the short term we:
1) Trust people not to load a second major LSM,
2) See below.

Quoted 128 lines

. We need to keep track of which module's inode_free_security() needs to be called

int security_inode_alloc(struct inode *inode)

{
        struct security_hook_list *P1;
        struct security_hook_list *P2;
        inode->i_security = NULL;
        hlist_for_each_entry(P1, &security_hook_heads_ro.inode_alloc_security, list) {
                int RC = P1->hook.inode_alloc_security(inode);
                if (RC != 0)
                        goto out;
        }
        hlist_for_each_entry(P1, &security_hook_heads_rw.inode_alloc_security, list) {
                int RC = P1->hook.inode_alloc_security(inode);
                if (RC != 0)
                        goto out;
        }
        return 0;
out:
        hlist_for_each_entry(P2, &security_hook_heads_ro.inode_free_security, list) {
                if (P1 == P2)
                        goto done;
                P2->hook.inode_free_security(inode);
        }
        hlist_for_each_entry(P2, &security_hook_heads_rw.inode_free_security, list) {
                if (P1 == P2)
                        goto done;
                P2->hook.inode_free_security(inode);
        }
done:
        return ret;
}

while handling race condition that foo->inode_alloc_security(inode) returned an error and foo is removed from the list and is waiting for SRCU grace period before hitting P1 == P2 check and therefore by error calls all LSM modules' ->inode_free_security(inode) hooks.

Quoted 22 lines

Quoted 22 lines

(quoted diff) +0 -18

@@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
 }
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
-static void lock_existing_hooks(void)
-{
-       struct lsm_info *info;
-
-       /*
-        * Prevent module unloading while we're doing this
-        * try_module_get may fail (safely), if the module
-        * is already unloading -- allow that.
-        */
-       mutex_lock(&module_mutex);
-       mutex_lock(&lsm_info_lock);
-       pr_info("Locking security hooks\n");
-
-       hlist_for_each_entry(info, &lsm_info_head, list)
-               WARN_ON(!try_module_get(info->owner));
-
-       mutex_unlock(&lsm_info_lock);
-       mutex_unlock(&module_mutex);

Quoted 8 lines

Review void security_delete_hooks(struct lsm_info *info) · line 159

Quoted 1 line

(quoted diff) +0 -1

-}

Why do you think we should remove this code? I think that it's valuable to keep in there because it prevents an administrator from accidentally panicing the system. For example, if you have admin A lock the security hooks, and then admin B comes along and tries to unload the module, IMHO they shouldn't get a panic, and by default (unless they rmmod -f) they should just a nice warning that suggests they're doing something wrong (like the module is in use).

What I suggested is

        /* Doing init or already dying? */
        if (mod->state != MODULE_STATE_LIVE) {
                /* FIXME: if (force), slam module count damn the torpedoes */
                pr_debug("%s already dying\n", mod->name);
                ret = -EBUSY;
                goto out;
        }

(quoted diff) +3 -0

+       ret = security_remove_module(mod);
+       if (ret)
+               goto out;
        /* If it has an init func, it must have an exit func to unload */
        if (mod->init && !mod->exit) {
                forced = try_force_unload(flags);

and check like

int security_remove_module(struct module *mod)

{
        struct lsm_info *info;
        int ret = 0;

        if (security_allow_unregister_hooks)
                return 0;

        mutex_lock(&lsm_info_lock);
        hlist_for_each_entry(info, &lsm_info_head, list)
                if (mod == info->owner)
                        ret = -EPERM;
        mutex_unlock(&lsm_info_lock);
        return ret;
}

rather than using register_module_notifier().

Quoted 5 lines

Thanks for the feedback. I think we're getting there at last.

I'd like Casey and James' feedback as well. I think the big questions are:
1) Can we assume that LSM authors will not shoot themselves in the foot and break major LSMs?

What "break major LSMs" means? Regardless of whether LKM-based LSMs use inode->i_security, we need to handle race condition described above.

Quoted 1 line

2) Should we protect the administrator from themselves (try_module_get)?

What I suggested is "not to play with module usage count".

Quoted 5 lines

3) Is it okay to allow hooks to be unloaded at all?

I think no matter what the answers to 1-3 are, we can apply patch 1, 2, and 3 once I include the fix-ups that you suggested here.

Quoted 6 lines

Quoted 2 lines

Patch 4 is heavily dependent on the answer to (1), and patches 6 is heavily dependent on the answer to (3).

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

What about something as stupid as:

security/security.c +2 -1

index 36065128c6c5..895bdb0b1381 100644
@@ -339,7 +339,8 @@ static void security_add_hook(struct
 security_hook_list *hook,
  * Each LSM has to register its hooks with the infrastructure.
  * Return 0 on success
  */
-int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable,
+                                       bool is_major)
 {
        struct security_hook_heads *heads;

        int i, ret = 0;

security/security.c +3 -0

@@ -348,6 +349,9 @@ int __must_check security_add_hooks(struct
 lsm_info *info, bool is_mutable)
        if (IS_ERR(heads))
                return PTR_ERR(heads);
 
+       if (!info->owner && is_major)
+               return -EPERM;
+
        if (mutex_lock_killable(&lsm_info_lock))
                return -EINTR;

OR

include/linux/lsm_hooks.h +8 -6

index 5c227bbb2883..9cec723e7cea 100644
@@ -2011,6 +2011,7 @@ struct lsm_info {
        const unsigned int              count;
        struct security_hook_list       *hooks;
        struct module                   *owner;
+       bool                            is_major;
 } __randomize_layout;
 
 struct security_hook_list {

@@ -2035,12 +2036,13 @@ struct security_hook_list {
                .hook   = { .HEAD = HOOK }      \
        }
 
-#define LSM_MODULE_INIT(NAME, HOOKS)           \
-       {                                       \
-               .name   = NAME,                 \
-               .hooks  = HOOKS,                \
-               .count  = ARRAY_SIZE(HOOKS),    \
-               .owner  = THIS_MODULE,          \
+#define LSM_MODULE_INIT(NAME, HOOKS, IS_MAJOR)         \
+       {                                               \
+               .name           = NAME,                 \
+               .hooks          = HOOKS,                \
+               .count          = ARRAY_SIZE(HOOKS),    \
+               .owner          = THIS_MODULE,          \
+               .is_major       = IS_MAJOR,             \
        }
 
 extern int __must_check security_add_hooks(struct lsm_info *lsm,

security/security.c +4 -0

index 36065128c6c5..a3bfe735c0e2 100644
@@ -80,6 +80,8 @@ void __security_delete_hooks(struct lsm_info *info)
 
        mutex_lock(&lsm_info_lock);
        hlist_del(&info->list);
+       if (info->is_major)
+               major_modules_loaded--;
        mutex_unlock(&lsm_info_lock);
 }
 #endif

@@ -331,6 +333,8 @@ static void security_add_hook(struct
 security_hook_list *hook,
        head = (struct hlist_head *)(heads) + idx;
        hlist_add_tail_rcu(&hook->list, head);
 }
+static int major_modules_loaded;
+
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.

* @lsm_info: The lsm_info struct for this security module

security/security.c +6 -0

@@ -351,6 +355,12 @@ int __must_check security_add_hooks(struct
 lsm_info *info, bool is_mutable)
        if (mutex_lock_killable(&lsm_info_lock))
                return -EINTR;
 
+       if (info->is_major && major_modules_loaded) {
+               ret = -EBUSY;
+               goto out;
+       }
+
+
        if (!atomic_read(&security_allow_unregister_hooks)) {
                /*

* Because hook deregistration is not allowed, we need to try

Quoted 5 lines

inode_free_security is called on all LSM callbacks -- so, it should be, as long as people don't load unloadable major LSMs. If we want to be super conservative here, we can do a try_module_get, and a module_put if RC != 0 on RW hooks. So, we could have something like call_int_hook_major, and call_void_hook_major.

Quoted 25 lines

Quoted 25 lines

        }
        /*** End of changes made by this series ***/
        return 0;
}

and what Casey's series is doing is

int security_inode_alloc(struct inode *inode)

{
        struct security_hook_list *P;
        inode->i_security = NULL;
        hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0) {
                        /*** Start of changes made by Casey's series ***/
                        hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
                                P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
                        }
                        /*** End of changes made by Casey's series ***/
                        return RC;
                }
        }
        return 0;
}

Quoted 5 lines

Right, but this could be taken care of by just ensuring that nobody allocates blobs (major LSM), and only one LSM returns a non-zero to the *alloc* callbacks? Right now, we have this because one has to be a "major" LSM in order to use these callbacks, and we ensure only one major LSM is active at a time.

Quoted 3 lines

I suggest that either in the short term we:
1) Trust people not to load a second major LSM,

Quoted 3 lines

2) See below.

What about something as stupid as:

One other aspect that may not be obvious is on the *_alloc_*, callbacks, we wouldn't want to call callback's of LSMs which are non-major.

Quoted 5 lines · Sargun Dhillon

Quoted 5 lines

inode_free_security is called on all LSM callbacks -- so, it should be, as long as people don't load unloadable major LSMs. If we want to be super conservative here, we can do a try_module_get, and a module_put if RC != 0 on RW hooks. So, we could have something like call_int_hook_major, and call_void_hook_major.

I don't think we want to play with module usage count.

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

        }
        /*** End of changes made by this series ***/
        return 0;
}

and what Casey's series is doing is

int security_inode_alloc(struct inode *inode)

{
        struct security_hook_list *P;
        inode->i_security = NULL;
        hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
                int RC = P->hook.inode_alloc_security(inode);
                if (RC != 0) {
                        /*** Start of changes made by Casey's series ***/
                        hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
                                P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
                        }
                        /*** End of changes made by Casey's series ***/
                        return RC;
                }
        }
        return 0;
}

Quoted 5 lines

Quoted 5 lines

Right, but this could be taken care of by just ensuring that nobody allocates blobs (major LSM), and only one LSM returns a non-zero to the *alloc* callbacks? Right now, we have this because one has to be a "major" LSM in order to use these callbacks, and we ensure only one major LSM is active at a time.

What Casey is trying to do is allow multiple major modules to use their own blob. Thus,

Quoted 3 lines

Quoted 3 lines

I suggest that either in the short term we:
1) Trust people not to load a second major LSM,

this is not an option.

Quoted 3 lines

Quoted 3 lines

2) See below.

What about something as stupid as:

I don't think we want to do this.

Quoted 3 lines

One other aspect that may not be obvious is on the *_alloc_*, callbacks, we wouldn't want to call callback's of LSMs which are non-major.

Quoted 6 lines · Tetsuo Handa

Sargun Dhillon wrote:
...

Quoted 2 lines

Quoted 2 lines

I suggest that either in the short term we:
1) Trust people not to load a second major LSM,

this is not an option.

This is exactly what people do today?

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

2) See below.

What about something as stupid as:

I don't think we want to do this.

We have the limit today of not allowing people to load two major LSMs. Why not wait till later to solve this problem, and for now, reject when people install two major LSMs? I think we can fix the dynamic loading problem _first_ and the multiple major LSM problem _second_

I think that we're on the verge of having a major merge collision.

That's a lot of overhead. The smaller the patches, the more work required to make the patch set. You're right that the current patches are too big. Moving forward I will be providing smaller, easier to deal with patches. The sheer overhead of rebaseing a large patch set has slowed down the development considerably. Minions. What I need are minions.

I hope to have the multiple major module code seriously reviewed as of 4.18 and start putting real pressure on getting it in for 4.19/4.20.

It is my firm hope that the multiple major module changes are going to start being seriously considered in the next release or two. That would reduce the complexity of what you're trying to accomplish because at the point all modules will be equal. I have always committed to making design choices that aren't going to prevent loadable/unloadable modules. I have also expressed no interest in doing it myself. From my selfish perspective I would like to see module loading follow my work, as having yet another major merge effort will delay the clean-up I know I'll have to do after 4.18.

The advent of the Age of Containers is driving this.

Quoted 1 line · Casey Schaufler

I think that we're on the verge of having a major merge collision.

I think that the reason of major merge collision is that your patchset is too big to review.

Quoted 7 lines

That's a lot of overhead. The smaller the patches, the more work required to make the patch set. You're right that the current patches are too big. Moving forward I will be providing smaller, easier to deal with patches. The sheer overhead of rebaseing a large patch set has slowed down the development considerably. Minions. What I need are minions.

You are holding the global lock for patchset which you did not get response. If you agree with breaking into smaller patches, we will be able to solve the major merge collision.

Quoted 2 lines

I hope to have the multiple major module code seriously reviewed as of 4.18 and start putting real pressure on getting it in for 4.19/4.20.

Before you repost your patchset, please answer questions below.

Q1: Apart from whether memory for security blobs should be managed by
infrastructure or by individual module, how do you guarantee that only release hook where corresponding alloc hook succeeded when one of alloc hooks returned an error?

My patch 1/2 provides that mechanism. Please show your patch which implements only that functionality instead of whole patchset.

Quoted 10 lines

It is my firm hope that the multiple major module changes are going to start being seriously considered in the next release or two. That would reduce the complexity of what you're trying to accomplish because at the point all modules will be equal. I have always committed to making design choices that aren't going to prevent loadable/unloadable modules. I have also expressed no interest in doing it myself. From my selfish perspective I would like to see module loading follow my work, as having yet another major merge effort will delay the clean-up I know I'll have to do after 4.18.

Depending on your patch, it is possible that managing memory for security blobs by infrastructure can become harmful for LKM-based LSMs, for the amount of memory which will be needed by LKM-based LSMs is not known until LKM-based LSMs are loaded.

LKM-based LSMs might be forced to manage their security blobs without using infrastructure-managed ->security field. Then, that will not make "all modules equal".

So, please show your approach which allows LKM-based LSMs to handle infrastructure-managed ->security field (if you want to use infrastructure-managed ->security field).

Quoted 1 line

The advent of the Age of Containers is driving this.

I've been asking for runtime loading of LSMs before the Age of Containers comes. ;-)

Q2: What functionality are Containers people want LSM?

Use different LSM module for different container?

Runtime load/unload of LSM modules?

Quoted 4 lines · Tetsuo Handa

Quoted 1 line · Casey Schaufler

I think that we're on the verge of having a major merge collision.

I think that the reason of major merge collision is that your patchset is too big to review.

My patch set is large because it has to address all of the aspects of multiple modules running on the system before some important members of the community will look at it seriously.

Quoted 11 lines

Quoted 7 lines

That's a lot of overhead. The smaller the patches, the more work required to make the patch set. You're right that the current patches are too big. Moving forward I will be providing smaller, easier to deal with patches. The sheer overhead of rebaseing a large patch set has slowed down the development considerably. Minions. What I need are minions.

You are holding the global lock for patchset which you did not get response. If you agree with breaking into smaller patches, we will be able to solve the major merge collision.

I will be presenting the set of smaller patches once I finish merging them into 4.18. It's a bigger jobs than usual because of the change from lists to hlists, the significant changes to AppArmor and the early stages of SELinux namespacing. I hope to have it done mid week, but the changes need to get tested, and there are lots of configurations involved.

I am perfectly amenable to the patches going in in logical stages. In fact, that is what I would recommend.

Quoted 8 lines

Quoted 2 lines

I hope to have the multiple major module code seriously reviewed as of 4.18 and start putting real pressure on getting it in for 4.19/4.20.

Before you repost your patchset, please answer questions below.

Q1: Apart from whether memory for security blobs should be managed by
infrastructure or by individual module, how do you guarantee that only release hook where corresponding alloc hook succeeded when one of alloc hooks returned an error?

The infrastructure allocates a zeroed blob. Module "alloc" hooks are called. The module specific hooks set their data as desired. If a module alloc hook fails the complete set of free hooks can be called because the module data will either have been set or will be still be zeroed. This means that module free hooks have to accommodate the possibility the the module specific data was never set.

The reality is that many of the free hooks go away with the infrastructure blob management. Those that remain can easily be confirmed to be safe in the case where all the modules data is NULL.

Quoted 2 lines

My patch 1/2 provides that mechanism. Please show your patch which implements only that functionality instead of whole patchset.

It's not an explicit change. It is an artifact of the infrastructure allocating and freeing the data. You can't tease the removal of the module free hooks apart from the infrastructure freeing the data.

Quoted 11 lines

Quoted 10 lines

It is my firm hope that the multiple major module changes are going to start being seriously considered in the next release or two. That would reduce the complexity of what you're trying to accomplish because at the point all modules will be equal. I have always committed to making design choices that aren't going to prevent loadable/unloadable modules. I have also expressed no interest in doing it myself. From my selfish perspective I would like to see module loading follow my work, as having yet another major merge effort will delay the clean-up I know I'll have to do after 4.18.

Quoted 4 lines

Depending on your patch, it is possible that managing memory for security blobs by infrastructure can become harmful for LKM-based LSMs, for the amount of memory which will be needed by LKM-based LSMs is not known until LKM-based LSMs are loaded.

It's certainly not more "harmful" than the single blob mechanism we have now. The patches I'm proposing don't make the situation any worse than it is today. As I've maintained all along, I'm not trying to address LKM-based modules.

There is no reason I can see that blob sizes couldn't change while a system is running (yes, I have looked into it) provided that the blob size and content list is recorded in the blob and that the dynamic modules check this information before trying to use it.

        smack_inode(struct inode *inode)
        {
                return inode->i_security + smack_blob_sizes.lbs_inode;
        }

        lkm_based_module_inode(struct inode *inode)
        {
                struct lsm_inode_blob_header *ibh = inode->security;
                if (some_appropriate_check(ibh, LKM_BASED_MODULE_ID))
                        return inode->i_security + lkm_based_module_blob_sizes.lbs_inode;
                /* No blob for this object */
                return NULL;
        }

A refinement to this might keep the offset for dynamic module data in the blob header. That would be more flexible, but would consume more memory for each blob.

Quoted 3 lines

LKM-based LSMs might be forced to manage their security blobs without using infrastructure-managed ->security field. Then, that will not make "all modules equal".

Regardless of how the data is managed the information about how much is being used and where to find it has to be kept somewhere. With static modules it is easier because you only need to figure that out once, at the beginning. If you're going to have blob data changing you have to look it up or figure it out at every reference. It's not impossible. It's not even really that hard, but it won't be cheap.

Quoted 3 lines

So, please show your approach which allows LKM-based LSMs to handle infrastructure-managed ->security field (if you want to use infrastructure-managed ->security field).

I'll repeat, I don't have code to do it because I have explicitly excluded lkm-based modules from my todo list. If I were doing it, I would propose an infrastructure managed header in the security blobs containing sufficient information for the modules to determine if the data they are looking for is present. lkm-based modules would have to be coded to accept that they might get blobs that were created before the module was loaded, and that do not contain data for that module.

The blob header mechanism would only be included if lkm-based security module support was configured.

Quoted 1 line

Quoted 1 line

The advent of the Age of Containers is driving this.

Quoted 2 lines

I've been asking for runtime loading of LSMs before the Age of Containers comes. ;-)

Yes, you have!

Quoted 1 line

Q2: What functionality are Containers people want LSM?

It varies. Container people are a diverse lot.

Quoted 1 line

Use different LSM module for different container?

That is definitely one use case. You might run Smack on the base system and label each container based on who payed for it, and let the security module set within the container be whatever the job demands.

Quoted 1 line

Runtime load/unload of LSM modules?

I would see that as something you might do on container startup, but managing the module set could get ugly.

Quoted 19 lines · Casey Schaufler

Quoted 4 lines · Tetsuo Handa

Quoted 1 line · Casey Schaufler

I think that we're on the verge of having a major merge collision.

I think that the reason of major merge collision is that your patchset is too big to review.

My patch set is large because it has to address all of the aspects of multiple modules running on the system before some important members of the community will look at it seriously.

Quoted 10 lines

Quoted 6 lines

That's a lot of overhead. The smaller the patches, the more work required to make the patch set. You're right that the current patches are too big. Moving forward I will be providing smaller, easier to deal with patches. The sheer overhead of rebaseing a large patch set has slowed down the development considerably. Minions. What I need are minions.

You are holding the global lock for patchset which you did not get response. If you agree with breaking into smaller patches, we will be able to solve the major merge collision.

Quoted 9 lines

I will be presenting the set of smaller patches once I finish merging them into 4.18. It's a bigger jobs than usual because of the change from lists to hlists, the significant changes to AppArmor and the early stages of SELinux namespacing. I hope to have it done mid week, but the changes need to get tested, and there are lots of configurations involved.

I am perfectly amenable to the patches going in in logical stages. In fact, that is what I would recommend.

I guess I'm just a little bit frustrated, because, in my mind, some of my patches provide immediate value, and are ready to be reviewed, and or respun. Testuo did a good job reviewing 1-5, and I think that if those are his only issues, then we're good to go.
I've:
1) Do safe hook loading for sane security modules (those that don't try to overload major hooks), without compromising the security of other hooks
2) Suggested a whitelist of major hooks to prevent insane modules from being loaded

If Casey's patches land, we can just not do (2), and we're good to go.

Quoted 1 line

Quoted 1 line

Quoted 1 line

...

One dumb use case I have is the ability to limit interactions with PTYs for containerized applications. Another aspect of this is being able to write policies in C, and actually being able to control the nitty gritty of what's going on, versus actively fighting with the LSM(s).

Quoted 40 lines · Sargun Dhillon

Quoted 19 lines · Casey Schaufler

Quoted 4 lines · Tetsuo Handa

Quoted 1 line · Casey Schaufler

I think that we're on the verge of having a major merge collision.

I think that the reason of major merge collision is that your patchset is too big to review.

My patch set is large because it has to address all of the aspects of multiple modules running on the system before some important members of the community will look at it seriously.

Quoted 10 lines

Quoted 6 lines

That's a lot of overhead. The smaller the patches, the more work required to make the patch set. You're right that the current patches are too big. Moving forward I will be providing smaller, easier to deal with patches. The sheer overhead of rebaseing a large patch set has slowed down the development considerably. Minions. What I need are minions.

You are holding the global lock for patchset which you did not get response. If you agree with breaking into smaller patches, we will be able to solve the major merge collision.

Quoted 9 lines

I will be presenting the set of smaller patches once I finish merging them into 4.18. It's a bigger jobs than usual because of the change from lists to hlists, the significant changes to AppArmor and the early stages of SELinux namespacing. I hope to have it done mid week, but the changes need to get tested, and there are lots of configurations involved.

I am perfectly amenable to the patches going in in logical stages. In fact, that is what I would recommend.

I guess I'm just a little bit frustrated, because, in my mind, some of my patches provide immediate value, and are ready to be reviewed, and or respun. Testuo did a good job reviewing 1-5, and I think that if those are his only issues, then we're good to go.
I've:
1) Do safe hook loading for sane security modules (those that don't try to overload major hooks), without compromising the security of other hooks
2) Suggested a whitelist of major hooks to prevent insane modules from being loaded

Quoted 1 line

If Casey's patches land, we can just not do (2), and we're good to go.

I understand your frustration. As much as I would like to avoid having the stacking work take any longer than it already has (I *do* have a boatload of other stuff I want to get done) I suggest that if you can get consensus that the lkm-based security module work should be accepted that you go forward and get it in. I know that I have at least one major round of review before the stacking can really be seriously considered. I knew the job was dangerous when I took it.

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

...

One dumb use case I have is the ability to limit interactions with PTYs for containerized applications. Another aspect of this is being able to write policies in C, and actually being able to control the nitty gritty of what's going on, versus actively fighting with the LSM(s).

You don't need lkm-based modules to do that, but I can see why you might want to do it that way. It's also something that I would expect LandLock to be a solution for, but again you'd need to be running LandLock when you identified the issue.

Quoted 42 lines · Casey Schaufler

Quoted 40 lines · Sargun Dhillon

Quoted 19 lines · Casey Schaufler

Quoted 4 lines · Tetsuo Handa

Quoted 1 line · Casey Schaufler

I think that we're on the verge of having a major merge collision.

I think that the reason of major merge collision is that your patchset is too big to review.

My patch set is large because it has to address all of the aspects of multiple modules running on the system before some important members of the community will look at it seriously.

Quoted 10 lines

Quoted 6 lines

That's a lot of overhead. The smaller the patches, the more work required to make the patch set. You're right that the current patches are too big. Moving forward I will be providing smaller, easier to deal with patches. The sheer overhead of rebaseing a large patch set has slowed down the development considerably. Minions. What I need are minions.

You are holding the global lock for patchset which you did not get response. If you agree with breaking into smaller patches, we will be able to solve the major merge collision.

Quoted 9 lines

I will be presenting the set of smaller patches once I finish merging them into 4.18. It's a bigger jobs than usual because of the change from lists to hlists, the significant changes to AppArmor and the early stages of SELinux namespacing. I hope to have it done mid week, but the changes need to get tested, and there are lots of configurations involved.

I am perfectly amenable to the patches going in in logical stages. In fact, that is what I would recommend.

I guess I'm just a little bit frustrated, because, in my mind, some of my patches provide immediate value, and are ready to be reviewed, and or respun. Testuo did a good job reviewing 1-5, and I think that if those are his only issues, then we're good to go.
I've:
1) Do safe hook loading for sane security modules (those that don't try to overload major hooks), without compromising the security of other hooks
2) Suggested a whitelist of major hooks to prevent insane modules from being loaded

Quoted 1 line

If Casey's patches land, we can just not do (2), and we're good to go.

Quoted 10 lines

I understand your frustration. As much as I would like to avoid having the stacking work take any longer than it already has (I *do* have a boatload of other stuff I want to get done) I suggest that if you can get consensus that the lkm-based security module work should be accepted that you go forward and get it in. I know that I have at least one major round of review before the stacking can really be seriously considered. I knew the job was dangerous when I took it.

Curious, consensus from whom? I thought you owned this part of the tree.

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

...

Do you know where Landlock is in terms of getting merged?

Quoted 2 lines

I've been a long-time proponent of programmable LSMs (before it was cool): https://lkml.org/lkml/2016/8/4/58

Happy to know that Landlock started before programmable access-control was cool, too. :)
https://lkml.kernel.org/r/1458784008-16277-1-git-send-email-mic at digikod.net

I guess I'm just a little bit frustrated, because, in my mind, some of my patches provide immediate value, and are ready to be reviewed, and or respun.

Quoted 3 lines · Sargun Dhillon

I guess I'm just a little bit frustrated, because, in my mind, some of my patches provide immediate value, and are ready to be reviewed, and or respun.

I'm not seeing much value in this functionality, given that SELinux is the only unloadable LSM, and that is really just an historical workaround which may be normalized at some point.

Patch 1 may be useful on its own.

--
James Morris
<[email protected]>

Quoted 6 lines · James Morris

Quoted 3 lines · Sargun Dhillon

I guess I'm just a little bit frustrated, because, in my mind, some of my patches provide immediate value, and are ready to be reviewed, and or respun.

Quoted 5 lines

I'm not seeing much value in this functionality, given that SELinux is the only unloadable LSM, and that is really just an historical workaround which may be normalized at some point.

Patch 1 may be useful on its own.

Do you not think patch 2 is also useful? Is it worth me re-rolling 1-2 independently?

Do you think not think that minor loadable LSMs are valuable? -- And if so, do you think it's okay with, or without guardrails?

Quoted 4 lines

It varies. Container people are a diverse lot.

Quoted 1 line

Use different LSM module for different container?

One dumb use case I have is the ability to limit interactions with PTYs for containerized applications. Another aspect of this is being able to write policies in C, and actually being able to control the nitty gritty of what's going on, versus actively fighting with the LSM(s).

The primary security benefit of this patchset is the introduction of read-only hooks, even if some security modules have mutable hooks. Currently, if you have any LSMs with mutable hooks it will render all heads, and list nodes mutable. These are a prime place to attack because being able to manipulate those hooks is a way to bypass all LSMs easily and to create a persistent, covert channel to intercept nearly all calls.

There is a shares SRCU between all security hooks to facilitate safe LSM- unloading. This SRCU is very cheap for runtime overhead on reads, but there is synchronization around it for unloads. There is only a cost to pay at unload time, which is based on the execution time of longest chain of callbacks after synchronization begins.

If CONFIG_SECURITY_WRITABLE_HOOKS is enabled, then hooks can be loaded at runtime. The module can check the return code of security_add_hooks to determine whether or not they can install their hooks, independently of checking for the Kconfig value. In the future, we may make it so that runtime hook loading is also disabled, after boot. We can do that in a follow-up patch.

If CONFIG_SECURITY_UNREGISTRABLE_HOOKS is enabled, then hooks can be unloaded at runtime. This behaviour can be disabled by setting security.allow_unregister_hooks to 0. Once set, it requires a reboot to be reset.

SELinux is exempt from the CONFIG_SECURITY_UNREGISTRABLE_HOOKS rule, and can unregister at any time. SELinux exposes a mechanism to disable itself, prior to policy loading. This can be disabled at compilation time, and it depends on CONFIG_SECURITY_WRITABLE_HOOKS. Changing this behaviour would require breaking the uapi.

This patch can allow for a new style of LSMs. There are many cases where LSM "policies" would be better defined in some formal programming language, like C. This is either due to flexibility, or performance, for functions in the hot path.

It also unlocks development usecases, but those come as a secondary benefit to the earlier feature. There has been an appetite for out-of-tree LSMs:
http://kernsec.org/pipermail/linux-security-module-archive/2017-March/000119.html
Casey's work has been great enabling minor LSM stacking, and a variety of LSMs have been proposed on list that haven't made it into the kernel but fit into the minor LSM stacking model.

One of the other big changes is the introduction of the lsm_info structure. This encapsulates various information about the LSM itself, and can be used at a later time.

This patch was tested with CONFIG_PROVE_LOCKING to prove the correctness of the locking in lock_existing_hooks. It grabs the module lock to prevent new hooks from being loaded, or current hooks from being unloaded. It then grabs the lsm_info_lock, which is only grabbed on hook modification, or inspection of the loaded LSMs.

The other thing that this introduces is monitoring of LKM unloading. If an LSM's LKM is unloaded when it is not supposed to be, it will crash the system.

Thanks to Tetsuo for their review, and feedback.

Changes since:
v6:
* introduce LSM info
* Crash by default instead of leaving it up to module authors to deal with things going wrong
* Monitor module unloading, to ensure that attempts to unload the LKM for an LSM crashes the system.
v5:
* Rename "CONFIG_SECURITY_UNLOADABLE_MODULES" to "CONFIG_SECURITY_UNREGISTRABLE_HOOKS" which enables arbitrary hook deregistration. If disabled, hooks are not allowed to deregister. SELinux is exempt.
* Rename security.allow_unload_hooks to security.allow_unregister_hooks
* Make it so that both security_add_hooks, and security_delete_hooks return errors upon being unsuccessful, allowing module authors to make the best decision for their use case.
* Get rid of LSM_HOOK_INIT_MUTABLE
v4:
* Introduce the configuration flag "CONFIG_SECURITY_UNLOADABLE_MODULES" to disable module unloading
* Introduce the kernel parameter security.allow_unload_hooks
v3:
* Instead of taking the approach of a "null hook", using the approach of a second set of hooks -- this was mostly done through the FOR_EACH_SECURITY_HOOK_MUTABLE macro, which gets compiled out if CONFIG_SECURITY_WRITABLE_HOOKS is disabled.
v2:
* Split out hlist_head patch
* Apply Tetsuo's changes to clean up functions which are not covered by call_int_hook / call_void_hook
* Disable NULL hook checking when uneeded
v1:
* Add SRCU to allow for code-unloading
* Add concurrency control around hook mutation

Sargun Dhillon (6):
security: Move LSM registration arguments to struct lsm_info
security: Make security_hook_heads private
security: Introduce mutable (RW) hooks
security: Expose security_add_hooks externally and add error handling
security: Panic on forced unloading of security module
security: Add SECURITY_UNREGISTRABLE_HOOKS to allow for hook removal

 include/linux/lsm_hooks.h                     |  67 ++--
 scripts/gcc-plugins/randomize_layout_plugin.c |   2 -
 security/Kconfig                              |  12 +
 security/apparmor/lsm.c                       |   8 +-
 security/commoncap.c                          |   8 +-
 security/inode.c                              |  56 +++-
 security/loadpin/loadpin.c                    |   6 +-
 security/security.c                           | 421 +++++++++++++++++++++-----
 security/security.h                           |  11 +
 security/selinux/hooks.c                      |  16 +-
 security/smack/smack_lsm.c                    |   5 +-
 security/tomoyo/tomoyo.c                      |   6 +-
 security/yama/yama_lsm.c                      |   6 +-
 13 files changed, 491 insertions(+), 133 deletions(-)
 create mode 100644 security/security.h

--
2.14.1

Quoted 4 lines · Sargun Dhillon

The primary security benefit of this patchset is the introduction of read-only hooks, even if some security modules have mutable hooks.
...

James, Casey,
Should I respin patches 1-5 with the fixes that Tetsuo suggested, and do you want to pick those up? It seems like other than the few errors, those look good. And then we can figure out to deal with patch 6 later?