[PATCH v4 2/3] security: Expose a mechanism to load lsm hooks dynamically at runtime

Sargun Dhillon sargun at sargun.me
Wed Mar 7 20:29:38 UTC 2018


On Wed, Mar 7, 2018 at 9:59 AM, Casey Schaufler <casey at schaufler-ca.com> wrote:
> On 3/6/2018 11:23 PM, Sargun Dhillon wrote:
>> This patch adds dynamic security hooks. These hooks are designed to allow
>> for safe runtime loading.
>>
>> These hooks are only run after all built-in, and major LSMs are run.
>> The LSMs enabled by this feature must be minor LSMs, but they can poke
>> at the security blobs, as the blobs should be initialized by the time
>> their callback happens.
>>
>> There should be little runtime performance overhead for this feature,
>> as it's protected behind static_keys, which are disabled by default,
>> and are only enabled per-hook at runtime, when a module is loaded.
>>
>> Currently, the hook heads are separated for dynamic hooks, because
>> it is not read-only like the hooks which are loaded at runtime.
>>
>> Some hooks are blacklisted, and attempting to load an LSM with any
>> of them in use will fail.
>>
>> Signed-off-by: Sargun Dhillon <sargun at sargun.me>
>> ---
>>  include/linux/lsm_hooks.h |  26 +++++-
>>  security/Kconfig          |   9 +++
>>  security/inode.c          |  13 ++-
>>  security/security.c       | 198 ++++++++++++++++++++++++++++++++++++++++++++--
>>  4 files changed, 234 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index d28c7f5b01c1..4e6351957dff 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -28,6 +28,7 @@
>>  #include <linux/security.h>
>>  #include <linux/init.h>
>>  #include <linux/rculist.h>
>> +#include <linux/module.h>
>>
>>  /**
>>   * union security_list_options - Linux Security Module hook function list
>> @@ -1968,6 +1969,9 @@ struct security_hook_list {
>>       enum lsm_hook                   head_idx;
>>       union security_list_options     hook;
>>       char                            *lsm;
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +     struct module                   *owner;
>> +#endif
>>  } __randomize_layout;
>>
>>  /*
>> @@ -1976,11 +1980,29 @@ struct security_hook_list {
>>   * care of the common case and reduces the amount of
>>   * text involved.
>>   */
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +#define LSM_HOOK_INIT(HEAD, HOOK)            \
>> +     {                                       \
>> +             .head_idx = HOOK_IDX(HEAD),     \
>> +             .hook = { .HEAD = HOOK },       \
>> +             .owner = THIS_MODULE,           \
>> +     }
>> +
>> +#else
>>  #define LSM_HOOK_INIT(HEAD, HOOK) \
>>       { .head_idx = HOOK_IDX(HEAD), .hook = { .HEAD = HOOK } }
>> +#endif
>>
>> -extern char *lsm_names;
>> -
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +extern int security_add_dynamic_hooks(struct security_hook_list *hooks,
>> +                                   int count, char *lsm);
>> +#else
>> +static inline int security_add_dynamic_hooks(struct security_hook_list *hooks,
>> +                                          int count, char *lsm)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>> +#endif
>>  extern void security_add_hooks(struct security_hook_list *hooks, int count,
>>                               char *lsm);
>>
>> diff --git a/security/Kconfig b/security/Kconfig
>> index c4302067a3ad..481b93b0d4d9 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -36,6 +36,15 @@ config SECURITY_WRITABLE_HOOKS
>>       bool
>>       default n
>>
>> +config SECURITY_DYNAMIC_HOOKS
>> +     bool "Runtime loadable (minor) LSMs via LKMs"
>> +     depends on SECURITY && SRCU
>> +     help
>> +       This enables LSMs which live in LKMs, and supports loading, and
>> +       unloading them safely at runtime. These LSMs must be minor LSMs.
>> +       They cannot circumvent the built-in LSMs.
>> +       If you are unsure how to answer this question, answer N.
>> +
>>  config SECURITYFS
>>       bool "Enable the securityfs filesystem"
>>       help
>> diff --git a/security/inode.c b/security/inode.c
>> index 8dd9ca8848e4..89be07b044a5 100644
>> --- a/security/inode.c
>> +++ b/security/inode.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/security.h>
>>  #include <linux/lsm_hooks.h>
>>  #include <linux/magic.h>
>> +#include <linux/mutex.h>
>> +
>> +extern char *lsm_names;
>> +extern struct mutex lsm_lock;
>>
>>  static struct vfsmount *mount;
>>  static int mount_count;
>> @@ -312,8 +316,13 @@ static struct dentry *lsm_dentry;
>>  static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
>>                       loff_t *ppos)
>>  {
>> -     return simple_read_from_buffer(buf, count, ppos, lsm_names,
>> -             strlen(lsm_names));
>> +     ssize_t ret;
>> +
>> +     mutex_lock(&lsm_lock);
>> +     ret = simple_read_from_buffer(buf, count, ppos, lsm_names,
>> +                                   strlen(lsm_names));
>> +     mutex_unlock(&lsm_lock);
>> +     return ret;
>>  }
>>
>>  static const struct file_operations lsm_ops = {
>> diff --git a/security/security.c b/security/security.c
>> index b9fb297b824e..492d44dd0549 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/backing-dev.h>
>>  #include <linux/string.h>
>>  #include <net/flow.h>
>> +#include <linux/mutex.h>
>>
>>  #define MAX_LSM_EVM_XATTR    2
>>
>> @@ -36,10 +37,18 @@
>>  #define SECURITY_NAME_MAX    10
>
> I expect that this limit will be too small for loadable modules.
>
We can change this. Any suggestion?

>>  static struct list_head security_hook_heads[__MAX_LSM_HOOK] __lsm_ro_after_init;
>> -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>> -
>>  #define HOOK_HEAD(NAME)      (&security_hook_heads[HOOK_IDX(NAME)])
>>
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +static struct list_head dynamic_security_hook_heads[__MAX_LSM_HOOK];
>> +static struct srcu_struct dynamic_hook_srcus[__MAX_LSM_HOOK];
>> +#define DYNAMIC_HOOK_HEAD(NAME)      (&dynamic_security_hook_heads[HOOK_IDX(NAME)])
>> +#define DYNAMIC_HOOK_SRCU(NAME)      (&dynamic_hook_srcus[HOOK_IDX(NAME)])
>> +DEFINE_STATIC_KEY_ARRAY_FALSE(dynamic_hook_keys, __MAX_LSM_HOOK);
>> +#endif
>> +static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>> +
>> +DEFINE_MUTEX(lsm_lock);
>>  char *lsm_names;
>>  /* Boot-time LSM user choice */
>>  static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
>> @@ -55,6 +64,23 @@ static void __init do_security_initcalls(void)
>>       }
>>  }
>>
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +static void security_init_dynamic_hooks(void)
>> +{
>> +     int i, err;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(dynamic_security_hook_heads); i++) {
>> +             INIT_LIST_HEAD(&dynamic_security_hook_heads[i]);
>> +             err = init_srcu_struct(&dynamic_hook_srcus[i]);
>> +             if (err)
>> +                     panic("%s: Could not initialize SRCU - %d\n",
>> +                           __func__, err);
>> +     }
>> +}
>> +#else
>> +static inline void security_init_dynamic_hooks(void) {};
>> +#endif
>> +
>>  /**
>>   * security_init - initializes the security framework
>>   *
>> @@ -66,6 +92,7 @@ int __init security_init(void)
>>
>>       for (i = 0; i < ARRAY_SIZE(security_hook_heads); i++)
>>               INIT_LIST_HEAD(&security_hook_heads[i]);
>> +     security_init_dynamic_hooks();
>>       pr_info("Security Framework initialized\n");
>>
>>       /*
>> @@ -172,6 +199,64 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>               panic("%s - Cannot get early memory.\n", __func__);
>>  }
>>
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +static int hook_allowed(enum lsm_hook hook_idx)
>> +{
>> +     switch (hook_idx) {
>> +     case HOOK_IDX(inode_getsecurity):
>> +     case HOOK_IDX(inode_setsecurity):
>> +     case HOOK_IDX(xfrm_state_pol_flow_match):
>> +             return -EPERM;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>
> Where did this list come from? Why doesn't it include
> secid_to_secctx? Why does it include xfrm_stat_pol_flow_match?
>
Because xfrm_stat_pol_flow_match is very perf-sensitive, and it's not
designed to work with multiple LSMs (as the comments say), similarly
with inode_getsecurity / inode_setsecurity. We can block other hooks,
but these were the ones in the code that had explicit comments.

>> +/**
>> + * security_add_dynamic_hooks:
>> + *   Register a dynamically loadable module's security hooks.
>> + *
>> + * @hooks: the hooks to add
>> + * @count: the number of hooks to add
>> + * @lsm: the name of the security module
>> + *
>> + * Returns:
>> + * 0 if successful
>> + * else errno, and no hooks will be installed
>> + */
>> +int security_add_dynamic_hooks(struct security_hook_list *hooks, int count,
>> +                             char *lsm)
>> +{
>> +     enum lsm_hook hook_idx;
>> +     int ret = 0, i;
>> +
>> +     for (i = 0; i < count; i++) {
>> +             ret = hook_allowed(hooks[i].head_idx);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     mutex_lock(&lsm_lock);
>> +     ret = lsm_append(lsm, &lsm_names);
>> +     if (ret)
>> +             goto out;
>> +
>> +     for (i = 0; i < count; i++) {
>> +             WARN_ON(!try_module_get(hooks[i].owner));
>> +             hooks[i].lsm = lsm;
>> +             hook_idx = hooks[i].head_idx;
>> +             list_add_tail_rcu(&hooks[i].list,
>> +                               &dynamic_security_hook_heads[hook_idx]);
>> +             static_branch_enable(&dynamic_hook_keys[hook_idx]);
>> +     }
>> +
>> +out:
>> +     mutex_unlock(&lsm_lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(security_add_dynamic_hooks);
>> +#endif
>> +
>>  int call_lsm_notifier(enum lsm_event event, void *data)
>>  {
>>       return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
>> @@ -200,14 +285,69 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
>>   *   This is a hook that returns a value.
>>   */
>>
>> -#define call_void_hook(FUNC, ...)                            \
>> +#define call_void_hook_builtin(FUNC, ...) do {                       \
>> +     struct security_hook_list *P;                           \
>> +     list_for_each_entry(P, HOOK_HEAD(FUNC), list)           \
>> +             P->hook.FUNC(__VA_ARGS__);                      \
>> +} while (0)
>> +
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +#define IS_DYNAMIC_HOOK_ENABLED(FUNC)                                \
>> +     (static_branch_unlikely(&dynamic_hook_keys[HOOK_IDX(FUNC)]))
>> +
>> +#define call_void_hook_dynamic(FUNC, ...) ({                 \
>> +     struct security_hook_list *P;                           \
>> +     int idx;                                                \
>> +                                                             \
>> +     idx = srcu_read_lock(DYNAMIC_HOOK_SRCU(FUNC));          \
>> +     list_for_each_entry_rcu(P,                              \
>> +                             DYNAMIC_HOOK_HEAD(FUNC),        \
>> +                             list) {                         \
>> +             P->hook.FUNC(__VA_ARGS__);                      \
>> +     }                                                       \
>> +     srcu_read_unlock(DYNAMIC_HOOK_SRCU(FUNC), idx);         \
>
> There has got to be a better way to do this than setting a lock
> for every hook call.
>
This isn't a "lock" -- it's rcu, so it should just do a ptr
dereference and per cpu counter inc / dec.

>> +})
>> +
>> +#define call_void_hook(FUNC, ...)    \
>> +     do {                            \
>> +             call_void_hook_builtin(FUNC, __VA_ARGS__);      \
>> +             if (!IS_DYNAMIC_HOOK_ENABLED(FUNC))             \
>> +                     break;                                  \
>
> Why not let the list macros do their job?
>
What do you mean? In order to avoid any perf overhead, this uses
static keys -- we have to check those.

>> +             call_void_hook_dynamic(FUNC, __VA_ARGS__);      \
>> +     } while (0)
>> +
>> +#define call_int_hook(FUNC, IRC, ...) ({                     \
>> +     bool continue_iteration = true;                         \
>> +     int RC = IRC, idx;                                      \
>>       do {                                                    \
>>               struct security_hook_list *P;                   \
>>                                                               \
>> -             list_for_each_entry(P, HOOK_HEAD(FUNC), list)   \
>> -                     P->hook.FUNC(__VA_ARGS__);              \
>> -     } while (0)
>> +             list_for_each_entry(P, HOOK_HEAD(FUNC), list) { \
>> +                     RC = P->hook.FUNC(__VA_ARGS__);         \
>> +                     if (RC != 0) {                          \
>> +                             continue_iteration = false;     \
>> +                             break;                          \
>> +                     }                                       \
>> +             }                                               \
>> +             if (!IS_DYNAMIC_HOOK_ENABLED(FUNC))             \
>> +                     break;                                  \
>> +             if (!continue_iteration)                        \
>> +                     break;                                  \
>> +             idx = srcu_read_lock(DYNAMIC_HOOK_SRCU(FUNC));  \
>> +             list_for_each_entry_rcu(P,                      \
>> +                                     DYNAMIC_HOOK_HEAD(FUNC), \
>> +                                     list) {                 \
>> +                     RC = P->hook.FUNC(__VA_ARGS__);         \
>> +                     if (RC != 0)                            \
>> +                             break;                          \
>> +             }                                               \
>> +             srcu_read_unlock(DYNAMIC_HOOK_SRCU(FUNC), idx); \
>> +     } while (0);                                            \
>> +     RC;                                                     \
>> +})
>>
>> +#else
>> +#define call_void_hook       call_void_hook_builtin
>
> I think this is hideous.
>
I can split this up.

>>  #define call_int_hook(FUNC, IRC, ...) ({                     \
>>       int RC = IRC;                                           \
>>       do {                                                    \
>> @@ -221,6 +361,7 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
>>       } while (0);                                            \
>>       RC;                                                     \
>>  })
>> +#endif
>>
>>  /* Security operations */
>>
>> @@ -312,6 +453,9 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>>       struct security_hook_list *hp;
>>       int cap_sys_admin = 1;
>>       int rc;
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +     int idx;
>> +#endif
>>
>>       /*
>>        * The module will respond with a positive value if
>> @@ -324,9 +468,25 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>>               rc = hp->hook.vm_enough_memory(mm, pages);
>>               if (rc <= 0) {
>>                       cap_sys_admin = 0;
>> -                     break;
>> +                     goto out;
>>               }
>>       }
>> +
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +     if (!IS_DYNAMIC_HOOK_ENABLED(vm_enough_memory))
>> +             goto out;
>> +     idx = srcu_read_lock(DYNAMIC_HOOK_SRCU(vm_enough_memory));
>> +     list_for_each_entry_rcu(hp, DYNAMIC_HOOK_HEAD(vm_enough_memory),
>> +                             list) {
>> +             rc = hp->hook.vm_enough_memory(mm, pages);
>> +             if (rc <= 0) {
>> +                     cap_sys_admin = 0;
>> +                     goto out;
>> +             }
>> +     }
>> +     srcu_read_unlock(DYNAMIC_HOOK_SRCU(vm_enough_memory), idx);
>> +#endif
>> +out:
>>       return __vm_enough_memory(mm, pages, cap_sys_admin);
>>  }
>>
>> @@ -1128,15 +1288,36 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>>       int thisrc;
>>       int rc = -ENOSYS;
>>       struct security_hook_list *hp;
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +     int idx;
>> +#endif
>>
>>       list_for_each_entry(hp, HOOK_HEAD(task_prctl), list) {
>>               thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
>>               if (thisrc != -ENOSYS) {
>>                       rc = thisrc;
>>                       if (thisrc != 0)
>> -                             break;
>> +                             goto out;
>>               }
>>       }
>> +
>> +#ifdef CONFIG_SECURITY_DYNAMIC_HOOKS
>> +     if (!IS_DYNAMIC_HOOK_ENABLED(task_prctl))
>> +             goto out;
>> +     idx = srcu_read_lock(DYNAMIC_HOOK_SRCU(task_prctl));
>> +     list_for_each_entry_rcu(hp, DYNAMIC_HOOK_HEAD(task_prctl),
>> +                             list) {
>> +             thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
>> +             if (thisrc != -ENOSYS) {
>> +                     rc = thisrc;
>> +                     if (thisrc != 0)
>> +                             goto out_unlock;
>> +             }
>> +     }
>> +out_unlock:
>> +     srcu_read_unlock(DYNAMIC_HOOK_SRCU(task_prctl), idx);
>> +#endif
>> +out:
>>       return rc;
>>  }
>>
>> @@ -1636,6 +1817,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>>               rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
>>               break;
>>       }
>> +
>>       return rc;
>>  }
>>
>
--
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