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

2018-04-01 Thread Tetsuo Handa
> +/*
> + * With writable hooks, we setup a structure like this:
> + * +--+   +---+   +---+   +---+   
> +--+
> + * |  |   |   |   |   |   |   |   |  
> |
> + * | HEAD +---> Immutable +---> Immutable +---> Null hook +---> Mutable Hook 
> |
> + * |  |   |  Hook 1   |   |  Hook 2   |   |   |   |  
> |
> + * +--+   +---+   +---+   +---+   
> +--+
> + *  |   ||
> + *  v   vv
> + *  CallbackCallback Callback
> + *
> + * The hooks before to null hook are marked only after kernel initialization.
> + * The null hook, as well as the hooks succeeding it are not marked read 
> only,
> + * therefore allowing them be (un)loaded after initialization time.
> + *
> + * Since the null hook doesn't have a callback, we need to check if a hook
> + * is the null hook prior to invoking it.
> + */

Do we need to use null hook as hook == NULL?
Why not overwrite null hook's hook field?

#define call_void_hook(FUNC, ...)   
\
do {
\
struct security_hook_list *P;   
\
int srcu_idx = lock_lsm();  
\
for (P = &security_hook_heads.FUNC; P->hook.FUNC; P = P->next)  
\
P->hook.FUNC(__VA_ARGS__);  
\
unlock_lsm(srcu_idx);
} while (0)

"struct hlist_head security_hook_heads[SECURITY_HOOK_COUNT]" is marked as 
__ro_after_init.
Built-in LSM module's "struct security_hook_list[]" is also marked as 
__ro_after_init.
Dynamic LSM module's "struct security_hook_list[]" is not marked as __initdata.

Hook registration function appends to tail of security_hook_heads.FUNC.
But, before __ro_after_init is applied, initial
"struct security_hook_list dynamic_hook_list[SECURITY_HOOK_COUNT]" is appended 
to
tail of security_hook_heads. That is, only "struct security_hook_list" at
initial dynamic_hook_list[] and later are writable.

Dynamic hook registration function overwrites current dynamic_hook_list[] with
supplied dynamic module's "struct security_hook_list[]". Then, dynamic hook
registration function allocates memory for next dynamic_hook_list[] and
appends to tail of security_hook_heads.FUNC (note that the tail element is
writable because it is guaranteed to be initial dynamic_hook_list[] or later.



Before registering first built-in immutable LSM module.

r/w
 * +--+
 * |  |
 * | HEAD +
 * |  |
 * +--+
 *
 *
 *

Before registering second built-in immutable LSM module.

r/wr/w
 * +--+   +---+
 * |  |   |   |
 * | HEAD +---> Immutable +
 * |  |   |  Hook 1   |
 * +--+   +---+
 *  |
 *  v
 *  Callback

Before registering initial dynamic_hook_list[].

r/wr/w r/w
 * +--+   +---+   +---+
 * |  |   |   |   |   |
 * | HEAD +---> Immutable +---> Immutable +
 * |  |   |  Hook 1   |   |  Hook 2   |
 * +--+   +---+   +---+
 *  |   |
 *  v   v
 *  CallbackCallback

After registering initial dynamic_hook_list[] and applying __ro_after_init.

r/or/o r/o r/w
 * +--+   +---+   +---+   ++
 * |  |   |   |   |   |   ||
 * | HEAD +---> Immutable +---> Immutable +---> Hook for first +
 * |  |   |  Hook 1   |   |  Hook 2   |   | Mutable Module |
 * +--+   +---+   +---+   ++
 *  |   |
 *  v   v
 *  CallbackCallback

After registering first mutable LSM module.

r/or/o r/o r/w r/w
 * +--+   +---+   +---+   +---+   
+-+
 * |  |   |   |   |   |   |   |   | 
|
 * | HEAD +---> Immutable +---> Immutable +---> Mutable   +---> Hook for second 
+
 * |  |   |  Hook 1   |   |  Hook 2   |   |  Hook 1   |   | Mutable Module  
|
 * +--+   +---+   +---+   +---+   
+-+
 *  |   | |
 *  v   v v
 *  CallbackCallback  Callback

After registering second mutable LSM module.

r/or/o r/o r/w r/w 
r/w
 * +--+   +---+   +---+   +--

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

2018-03-31 Thread Casey Schaufler
On 3/30/2018 11:16 PM, Sargun Dhillon wrote:
> On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler  
> wrote:
>> On 3/29/2018 7:33 PM, Sargun Dhillon wrote:
>>> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
 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 
> Signed-off-by: Tetsuo Handa 
> ---
>  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);
>  }
>
>

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

2018-03-31 Thread Kees Cook
On Fri, Mar 30, 2018 at 11:16 PM, Sargun Dhillon  wrote:
> On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler  
> wrote:
>>>  static struct security_hook_list null_hooks[SECURITY_HOOK_COUNT];
>>> -#define HAS_FUNC(SHL, FUNC)  (SHL->hook.FUNC)
>>> +DEFINE_STATIC_SRCU(security_hook_srcu);
>>> +
>>> +static inline bool is_null_hook(struct security_hook_list *shl)
>>> +{
>>> + union {
>>> + void *cb_ptr;
>>> + union security_list_options slo;
>>> + } hook_options;
>>> +
>>> + hook_options.slo = shl->hook;
>>> + return !hook_options.cb_ptr;
>>> +}
>>
>> I like the HAS_FUNC() approach better.
>
>  Just curious, why? I personally prefer small static inline functions
> over macros, if possible.

Generally speaking, small static inline functions are better since
they provide type-checking. In this case, though, it looks like you're
just doing a cast, but with a union. Why isn't this just:

return !!((uintptr_t)shl->hook)

?

Though the security_list_options union exists for callback type
checking, so really, having HAS_FUNC() with the explicit function
you're interested in creates a bit of self-documenting code (even if
it always resolves to the above test).

-Kees

-- 
Kees Cook
Pixel Security


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

2018-03-30 Thread Sargun Dhillon
On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler  wrote:
> On 3/29/2018 7:33 PM, Sargun Dhillon wrote:
>> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
>>> 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 
 Signed-off-by: Tetsuo Handa 
 ---
  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
>>

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

2018-03-30 Thread Casey Schaufler
On 3/29/2018 7:33 PM, Sargun Dhillon wrote:
> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
>> 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 
>>> Signed-off-by: Tetsuo Handa 
>>> ---
>>>  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 
>>>  #include 
>>>  #include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define SECURITY_HOOK_COUNT \
>

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

2018-03-29 Thread Sargun Dhillon
On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
> 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 
> > Signed-off-by: Tetsuo Handa 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +
> > +#define SECURITY_HOOK_COUNT \
> > +   (sizeof(security_hook_heads) / sizeof(st

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

2018-03-29 Thread 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 
> Signed-off-by: Tetsuo Handa 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
> +
> +#define SECURITY_HOOK_COUNT \
> + (sizeof(security_hook_heads) / sizeof(struct hlist_head))
>  
>  #define MAX_LSM_EVM_XATTR2
>  
> @@ -36,7 +41,10 @@
>  #define SECURITY_NAME_MAX10
>  
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> +EXPORT_SYMBOL_GPL(security_hook_heads);
> +
>  static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain)