diff mbox

[v7,0/6] Safe LSM (un)loading, and immutable hooks

Message ID 201804260715.w3Q7FOlb036047@www262.sakura.ne.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Tetsuo Handa April 26, 2018, 7:15 a.m. UTC
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(-)

Comments

Sargun Dhillon April 26, 2018, 7:41 a.m. UTC | #1
On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> 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.

> ---
>  include/linux/lsm_hooks.h |  4 +--
>  security/security.c       | 86 +++++++++++++----------------------------------
>  security/security.h       |  5 ---
>  3 files changed, 25 insertions(+), 70 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 5c227bb..98809d6 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -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);
> diff --git a/security/security.c b/security/security.c
> index 3606512..a4bc9cd 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -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
>   * indicating failure.
> - *
> - * When this value turns true -> false -- Once, lock_existing_hooks will be
> - * called.
>   */
> -static atomic_t security_allow_unregister_hooks =
> -       ATOMIC_INIT(IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0);
> +static bool security_allow_unregister_hooks =
> +       IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0;
>
If this is a static bool, can't it just be IS_ENABLED?

>  #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
> @@ -128,9 +125,8 @@ static inline void unlock_lsm(int idx)
>   */
>  void security_delete_hooks(struct lsm_info *info)
>  {
> -       if (!atomic_read(&security_allow_unregister_hooks))
> -               panic("Security module %s is attempting to delete hooks.\n",
> -                       info->name);
> +       if (!security_allow_unregister_hooks)
> +               panic("Security hooks are already locked.\n");
I don't understand the benefit of changing the language to this. Why
not something like panic("Security module is attempted to delete
locked hooks")? Otherwise, it can be misconstrued that this method
actually locks hooks. Also, given you have the same error phrase
everywhere, should it just be a #define?
>
>         __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.
> -        */
> -       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);
> -}
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).

> -
>  static int allow_unregister_hooks_set(const char *val,
>                                         const struct kernel_param *kp)
>  {
> -       const int current_val = atomic_read(&security_allow_unregister_hooks);
> +       const bool current_val = security_allow_unregister_hooks;
Do we need READ_ONCE here to prevent this from being read multiple
times in the function?

>         bool new_val;
> -       int ret;
>
> -       ret = strtobool(val, &new_val);
> -       if (ret)
> +       if (strtobool(val, &new_val))
>                 return 0;
I think I actually made an error here -- and it should have read
+       ret = strtobool(val, &new_val);
+       if (ret)
+               return ret;

Otherwise the user would not get an error if they did something like
echo "badvalue" into it.


>
>         /* 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;
>
> -       if (val != MODULE_STATE_GOING)
> +       if (val != MODULE_STATE_GOING || security_allow_unregister_hooks)
>                 return NOTIFY_DONE;
>
>         mutex_lock(&lsm_info_lock);
>         hlist_for_each_entry(info, &lsm_info_head, list)
>                 if (mod == info->owner)
> -                       panic("Security module %s is being unloaded without deregistering hooks",
> -                               info->name);
> +                       panic("Security hooks are already locked.\n");
See above.

>         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);
>
> diff --git a/security/security.h b/security/security.h
> index 3e757f5..ed056d5 100644
> --- a/security/security.h
> +++ b/security/security.h
> @@ -1,11 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> -#include <linux/lsm_hooks.h>
>
> -#ifndef __SECURITY_SECURITY_H
> -#define __SECURITY_SECURITY_H
Why get rid of these?
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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).
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tetsuo Handa April 26, 2018, 12:07 p.m. UTC | #2
Sargun Dhillon wrote:
> On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > 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.

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.



> > @@ -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);
> > -}
> 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;
 	}
 
+	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().



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

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

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

> 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).
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c227bb..98809d6 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -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);
diff --git a/security/security.c b/security/security.c
index 3606512..a4bc9cd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -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
  * indicating failure.
- *
- * When this value turns true -> false -- Once, lock_existing_hooks will be
- * called.
  */
-static atomic_t security_allow_unregister_hooks =
-	ATOMIC_INIT(IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0);
+static bool security_allow_unregister_hooks =
+	IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0;
 
 #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
@@ -128,9 +125,8 @@  static inline void unlock_lsm(int idx)
  */
 void security_delete_hooks(struct lsm_info *info)
 {
-	if (!atomic_read(&security_allow_unregister_hooks))
-		panic("Security module %s is attempting to delete hooks.\n",
-			info->name);
+	if (!security_allow_unregister_hooks)
+		panic("Security hooks are already locked.\n");
 
 	__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.
-	 */
-	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);
-}
-
 static int allow_unregister_hooks_set(const char *val,
 					const struct kernel_param *kp)
 {
-	const int current_val = atomic_read(&security_allow_unregister_hooks);
+	const bool current_val = security_allow_unregister_hooks;
 	bool new_val;
-	int ret;
 
-	ret = strtobool(val, &new_val);
-	if (ret)
+	if (strtobool(val, &new_val))
 		return 0;
 
 	/* 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;
 
-	if (val != MODULE_STATE_GOING)
+	if (val != MODULE_STATE_GOING || security_allow_unregister_hooks)
 		return NOTIFY_DONE;
 
 	mutex_lock(&lsm_info_lock);
 	hlist_for_each_entry(info, &lsm_info_head, list)
 		if (mod == info->owner)
-			panic("Security module %s is being unloaded without deregistering hooks",
-				info->name);
+			panic("Security hooks are already locked.\n");
 	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);
 
diff --git a/security/security.h b/security/security.h
index 3e757f5..ed056d5 100644
--- a/security/security.h
+++ b/security/security.h
@@ -1,11 +1,6 @@ 
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/mutex.h>
 #include <linux/list.h>
-#include <linux/lsm_hooks.h>
 
-#ifndef __SECURITY_SECURITY_H
-#define __SECURITY_SECURITY_H
 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