Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Casey Schaufler]

On 3/29/2018 2:14 PM, Sargun Dhillon wrote:
> This patch introduces a mechanism to add mutable hooks and immutable
> hooks to the callback chain. It adds an intermediary item to the
> chain which separates mutable and immutable hooks. Immutable hooks
> are then marked as read-only, as well as the hook heads. This does
> not preclude some hooks being able to be mutated (removed).
>
> It also wraps the hook unloading, and execution with an SRCU. One
> SRCU is used across all hooks, as the SRCU struct can be memory
> intensive, and hook execution time in general should be relatively
> short.
>
> Signed-off-by: Sargun Dhillon <sargun@xxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
>  include/linux/lsm_hooks.h  |  23 ++---
>  security/Kconfig           |   2 +-
>  security/apparmor/lsm.c    |   2 +-
>  security/commoncap.c       |   2 +-
>  security/security.c        | 210 ++++++++++++++++++++++++++++++++++++++-------
>  security/selinux/hooks.c   |   5 +-
>  security/smack/smack_lsm.c |   3 +-
>  security/tomoyo/tomoyo.c   |   3 +-
>  security/yama/yama_lsm.c   |   2 +-
>  9 files changed, 196 insertions(+), 56 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 09bc60fb35f1..689e5e72fb38 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1981,9 +1981,12 @@ extern struct security_hook_heads security_hook_heads;
>  extern char *lsm_names;
>  
>  extern void security_add_hooks(struct security_hook_list *hooks, int count,
> -				char *lsm);
> +				char *lsm, bool is_mutable);
>  
> -#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +#define __lsm_ro_after_init	__ro_after_init
> +/* Currently required to handle SELinux runtime hook disable. */
> +#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
> +#define __lsm_mutable_after_init
>  /*
>   * Assuring the safety of deleting a security module is up to
>   * the security module involved. This may entail ordering the
> @@ -1996,21 +1999,9 @@ extern void security_add_hooks(struct security_hook_list *hooks, int count,
>   * disabling their module is a good idea needs to be at least as
>   * careful as the SELinux team.
>   */
> -static inline void security_delete_hooks(struct security_hook_list *hooks,
> -						int count)
> -{
> -	int i;
> -
> -	for (i = 0; i < count; i++)
> -		hlist_del_rcu(&hooks[i].list);
> -}
> -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
> -
> -/* Currently required to handle SELinux runtime hook disable. */
> -#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
> -#define __lsm_ro_after_init
> +extern void security_delete_hooks(struct security_hook_list *hooks, int count);
>  #else
> -#define __lsm_ro_after_init	__ro_after_init
> +#define __lsm_mutable_after_init __ro_after_init
>  #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
>  
>  extern int __init security_module_enable(const char *module);
> diff --git a/security/Kconfig b/security/Kconfig
> index c4302067a3ad..a3b8b1142e6f 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -32,7 +32,7 @@ config SECURITY
>  	  If you are unsure how to answer this question, answer N.
>  
>  config SECURITY_WRITABLE_HOOKS
> -	depends on SECURITY
> +	depends on SECURITY && SRCU
>  	bool
>  	default n
>  
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 9a65eeaf7dfa..d6cca8169df0 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1155,7 +1155,7 @@ static int __init apparmor_init(void)
>  		goto buffers_out;
>  	}
>  	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> -				"apparmor");
> +				"apparmor", false);
>  
>  	/* Report that AppArmor successfully initialized */
>  	apparmor_initialized = 1;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 48620c93d697..fe4b0d9d44ce 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1363,7 +1363,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>  void __init capability_add_hooks(void)
>  {
>  	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
> -				"capability");
> +				"capability", false);
>  }
>  
>  #endif /* CONFIG_SECURITY */
> diff --git a/security/security.c b/security/security.c
> index 3cafff61b049..2ddb64864e3e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -29,6 +29,11 @@
>  #include <linux/backing-dev.h>
>  #include <linux/string.h>
>  #include <net/flow.h>
> +#include <linux/srcu.h>
> +#include <linux/mutex.h>
> +
> +#define SECURITY_HOOK_COUNT \
> +	(sizeof(security_hook_heads) / sizeof(struct hlist_head))
>  
>  #define MAX_LSM_EVM_XATTR	2
>  
> @@ -36,7 +41,10 @@
>  #define SECURITY_NAME_MAX	10
>  
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> +EXPORT_SYMBOL_GPL(security_hook_heads);
> +
>  static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> +static DEFINE_MUTEX(security_hook_mutex);
>  
>  char *lsm_names;
>  /* Boot-time LSM user choice */
> @@ -53,6 +61,103 @@ static void __init do_security_initcalls(void)
>  	}
>  }
>  
> +#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
> +DEFINE_STATIC_SRCU(security_hook_srcu);
> +static struct security_hook_list	null_hooks[SECURITY_HOOK_COUNT];
> +#define HAS_FUNC(SHL, FUNC)	(SHL->hook.FUNC)

The HAS_FUNC() macro will work, but it's awkward outside of the
call_..._hook() macros. I think you should document how to use it
properly somewhere in here. There are enough cases where the
call_..._hook() macros aren't used that someone could have trouble
figuring out how to use it.