[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks
Sargun Dhillon
sargun at sargun.me
Thu Apr 26 07:41:44 UTC 2018
On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
<penguin-kernel at 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 at 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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the Linux-security-module-archive
mailing list