Re: [RFC/PATCH 2/3] security: Add the Timgad module

[Djalal Harouni]

On Sat, Feb 11, 2017 at 1:33 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Thu, Feb 2, 2017 at 9:04 AM, Djalal Harouni <tixxdz@xxxxxxxxx> wrote:
>> From: Djalal Harouni <tixxdz@xxxxxxxxx>
>>
>> This adds the Timgad module. Timgad allows to apply restrictions on
>> which task is allowed to load or unload kernel modules. Auto-load module
>> feature is also handled. The settings can also be applied globally using
>> a sysctl interface, this allows to complete the core kernel interface
>> "modules_disable" which has only two modes: allow globally or deny
>> globally.
>
> To bikeshed on the name: since this is a module loading restriction
> LSM, perhaps something more descriptive: ModAutoRestrict or something
> like that? (Yes, Yama is poorly named, but initially it was going to
> be more than just ptrace restrictions...)

Yes, same thing here, I was also thinking maybe I can add an other
restriction here if necessary that apply using prctl() interface (I've
another case need to discuss it first). It would make little sense to
add another module for one prctl() use case and have to duplicate
bookkeeping of task<->security blob where there is already some
infrastructure. I also read your response and Casey's security blob
stacking work so yes lets see, otherwise I will update the name to
ModAutoRestrict or another one, no problem ;-)


>> The feature is useful for sandboxing, embedded systems and Linux
>> containers where only some containers/processes that have the
>> right privileges are allowed to load/unload modules. Unprivileged
>
> I'd be explicit here and discuss _auto_loading of modules. (Otherwise
> people quickly get confused about this vs CAP_SYS_MODULE.) You mention
> auto-load later, but I think mentioning it first make this easier to
> understand.

Ok.

>> processes should not be able to load/unload modules nor trigger the
>> module auto-load feature. This behaviour was inspired from grsecurity's
>> GRKERNSEC_MODHARDEN option.
>>
>> However I still did not complete the check since this has to be
>> discussed first, so any bug here is not from grsecurity, but my bugs and
>> on purpose. As this is a preliminary RFC these points are not settled,
>> discussion has to happen on what should be the best behaviour and what
>> checks should be in place. Currently the settings:
>>
>> Timgad module can be controled using a global sysctl setting:
>
> typo: controlled
>
>>    /proc/sys/kernel/timgad/module_restrict
>
> If this becomes /proc/sys/kernel/mod_autoload_restrict/ then the flag
> can just be "enabled". (e.g. see LoadPin LSM.)

I see but I guess we need at least 3 values.

>> Or using the prctl() interface:
>>    prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
>>
>> *) The per-process prctl() settings are:
>>     prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
>
> Excellent, yes, please require the trailing zeros. :)
>
>>     Where value means:
>>
>> 0 - Classic module load and unload permissions, nothing changes.
>
> "Classic module auto-load permissions ..." Nothing can auto-unload as-is.
>
>> 1 - The current process must have CAP_SYS_MODULE to be able to load and
>>     unload modules. CAP_NET_ADMIN should allow the current process to
>>     load and unload only netdev aliased modules, not implemented
>
> "... to be able to auto-load modules." Same thing about unloading:
> everything needs CAP_SYS_MODULE to unload a module.
>
>> 2 - Current process can not loaded nor unloaded modules.
>
> "... cannot auto-load modules."

Ok.


>>
>> *) sysctl interface supports the followin values:
>>
>> 0 - Classic module load and unload permissions, nothing changes.
>>
>> 1 - Only privileged processes with CAP_SYS_MODULE should be able to load and
>>     unload modules.
>>
>>     To be added: processes with CAP_NET_ADMIN should be able to
>>     load and unload only netdev aliased modules, this is currently not
>>     supported. Other checks for real root without CAP_SYS_MODULE ? ...
>>
>>     (This should be improved)
>>
>> 2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
>>      cannot be changed.
>>
>> Rules:
>> First the prctl() settings are checked, if the access is not denied
>> then the global sysctl settings are checked.
>>
>> As said I will update the permission checks later, this is a preliminary
>> RFC.
>>
>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> Signed-off-by: Djalal Harouni <tixxdz@xxxxxxxxx>
>> ---
>>  include/linux/lsm_hooks.h     |   5 +
>>  include/uapi/linux/prctl.h    |   5 +
>>  security/Kconfig              |   1 +
>>  security/Makefile             |   2 +
>>  security/security.c           |   1 +
>>  security/timgad/Kconfig       |  10 ++
>>  security/timgad/Makefile      |   3 +
>>  security/timgad/timgad_core.c | 306 +++++++++++++++++++++++++++++++++++++++
>>  security/timgad/timgad_core.h |  53 +++++++
>>  security/timgad/timgad_lsm.c  | 327 ++++++++++++++++++++++++++++++++++++++++++
>>  10 files changed, 713 insertions(+)
>>  create mode 100644 security/timgad/Kconfig
>>  create mode 100644 security/timgad/Makefile
>>  create mode 100644 security/timgad/timgad_core.c
>>  create mode 100644 security/timgad/timgad_core.h
>>  create mode 100644 security/timgad/timgad_lsm.c
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index b37e35e..6b83aaa 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -1935,5 +1935,10 @@ void __init loadpin_add_hooks(void);
>>  #else
>>  static inline void loadpin_add_hooks(void) { };
>>  #endif
>> +#ifdef CONFIG_SECURITY_TIMGAD
>> +extern void __init timgad_add_hooks(void);
>> +#else
>> +static inline void __init timgad_add_hooks(void) { }
>> +#endif
>>
>>  #endif /* ! __LINUX_LSM_HOOKS_H */
>> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
>> index a8d0759..6d80eed 100644
>> --- a/include/uapi/linux/prctl.h
>> +++ b/include/uapi/linux/prctl.h
>> @@ -197,4 +197,9 @@ struct prctl_mm_map {
>>  # define PR_CAP_AMBIENT_LOWER          3
>>  # define PR_CAP_AMBIENT_CLEAR_ALL      4
>>
>> +/* Control Timgad LSM options */
>> +#define PR_TIMGAD_OPTS                 48
>> +# define PR_TIMGAD_SET_MOD_RESTRICT    1
>> +# define PR_TIMGAD_GET_MOD_RESTRICT    2
>> +
>>  #endif /* _LINUX_PRCTL_H */
>> diff --git a/security/Kconfig b/security/Kconfig
>> index 118f454..e6d6128 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -164,6 +164,7 @@ source security/tomoyo/Kconfig
>>  source security/apparmor/Kconfig
>>  source security/loadpin/Kconfig
>>  source security/yama/Kconfig
>> +source security/timgad/Kconfig
>>
>>  source security/integrity/Kconfig
>>
>> diff --git a/security/Makefile b/security/Makefile
>> index f2d71cd..6a8127e 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
>>  subdir-$(CONFIG_SECURITY_APPARMOR)     += apparmor
>>  subdir-$(CONFIG_SECURITY_YAMA)         += yama
>>  subdir-$(CONFIG_SECURITY_LOADPIN)      += loadpin
>> +subdir-$(CONFIG_SECURITY_TIMGAD)       += timgad
>>
>>  # always enable default capabilities
>>  obj-y                                  += commoncap.o
>> @@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_APPARMOR)               += apparmor/
>>  obj-$(CONFIG_SECURITY_YAMA)            += yama/
>>  obj-$(CONFIG_SECURITY_LOADPIN)         += loadpin/
>>  obj-$(CONFIG_CGROUP_DEVICE)            += device_cgroup.o
>> +obj-$(CONFIG_SECURITY_TIMGAD)          += timgad/
>>
>>  # Object integrity file lists
>>  subdir-$(CONFIG_INTEGRITY)             += integrity
>> diff --git a/security/security.c b/security/security.c
>> index 5c699c8..c6c9349 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -61,6 +61,7 @@ int __init security_init(void)
>>         capability_add_hooks();
>>         yama_add_hooks();
>>         loadpin_add_hooks();
>> +       timgad_add_hooks();
>>
>>         /*
>>          * Load all the remaining security modules.
>> diff --git a/security/timgad/Kconfig b/security/timgad/Kconfig
>> new file mode 100644
>> index 0000000..93ecdb6
>> --- /dev/null
>> +++ b/security/timgad/Kconfig
>> @@ -0,0 +1,10 @@
>> +config SECURITY_TIMGAD
>> +       bool "TIMGAD support"
>> +       depends on SECURITY
>> +       default n
>> +       help
>> +         This selects TIMGAD, which applies restrictions on load and unload
>> +          modules operations. Further information can be found in
>> +          Documentation/security/Timgad.txt.
>
> I'd select a capitalization and stick with it (timgad, Timgad,
> TIMGAD?) modulo its renaming, etc.

Noted.

>> +
>> +         If you are unsure how to answer this question, answer N.
>> diff --git a/security/timgad/Makefile b/security/timgad/Makefile
>> new file mode 100644
>> index 0000000..dca0441
>> --- /dev/null
>> +++ b/security/timgad/Makefile
>> @@ -0,0 +1,3 @@
>> +obj-$(CONFIG_SECURITY_TIMGAD) := timgad.o
>> +
>> +timgad-y := timgad_core.o timgad_lsm.o
>> diff --git a/security/timgad/timgad_core.c b/security/timgad/timgad_core.c
>> new file mode 100644
>> index 0000000..fa35965
>> --- /dev/null
>> +++ b/security/timgad/timgad_core.c
>> [...]
>
> In the hopes of reusing the task stacking stuff from Casey, I'm going
> to skip reading the above code at least for now. ;)

Ok :-) , also on another note a quick test with RCU readers, a lookup
on more than 15000 tasks with a timgad context:
    11.33%  ps           [kernel.kallsyms]        [k] lock_acquire
    10.72%  bash         [kernel.kallsyms]        [k]
page_check_address_transhuge
     8.11%  ps           [kernel.kallsyms]        [k]
_raw_spin_unlock_irqrestore
     6.52%  ps           [kernel.kallsyms]        [k] lock_release
     3.45%  ps           [kernel.kallsyms]        [k] lock_is_held_type
     ...
     0.01%  bash         [kernel.kallsyms]        [k] get_timgad_task
     0.01%  bash         [kernel.kallsyms]        [k] getname_flags
     0.01%  bash         [kernel.kallsyms]        [k] hashtab_search
     0.01%  bash         [kernel.kallsyms]        [k] iput


>> diff --git a/security/timgad/timgad_core.h b/security/timgad/timgad_core.h
>> new file mode 100644
>> index 0000000..4096c39
>> --- /dev/null
>> +++ b/security/timgad/timgad_core.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Timgad Linux Security Module
>> + *
>> + * Author: Djalal Harouni
>> + *
>> + * Copyright (c) 2017 Djalal Harouni
>> + * Copyright (c) 2017 Endocode AG
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#define TIMGAD_MODULE_OFF      0x00000000
>> +#define TIMGAD_MODULE_STRICT   0x00000001
>> +#define TIMGAD_MODULE_NO_LOAD  0x00000002
>> +
>> +struct timgad_task;
>> +
>> +static inline int timgad_op_to_flag(unsigned long op,
>> +                                   unsigned long value,
>> +                                   unsigned long *flag)
>> +{
>> +       if (op != PR_TIMGAD_SET_MOD_RESTRICT || value > TIMGAD_MODULE_NO_LOAD)
>> +               return -EINVAL;
>> +
>> +       *flag = value;
>> +       return 0;
>> +}
>> +
>> +unsigned long read_timgad_task_flags(struct timgad_task *timgad_tsk);
>> +
>> +int timgad_task_set_op_flag(struct timgad_task *timgad_tsk,
>> +                           unsigned long op, unsigned long flag,
>> +                           unsigned long value);
>> +
>> +int is_timgad_task_op_set(struct timgad_task *timgad_tsk, unsigned long op,
>> +                         unsigned long *flag);
>> +
>> +struct timgad_task *get_timgad_task(struct task_struct *tsk);
>> +void put_timgad_task(struct timgad_task *timgad_tsk, bool *collect);
>> +struct timgad_task *lookup_timgad_task(struct task_struct *tsk);
>> +
>> +void release_timgad_task(struct task_struct *tsk);
>> +
>> +struct timgad_task *init_timgad_task(struct task_struct *tsk,
>> +                                    unsigned long flag);
>> +struct timgad_task *give_me_timgad_task(struct task_struct *tsk,
>> +                                       unsigned long value);
>> +
>> +int timgad_tasks_init(void);
>> +void timgad_tasks_clean(void);
>> diff --git a/security/timgad/timgad_lsm.c b/security/timgad/timgad_lsm.c
>> new file mode 100644
>> index 0000000..809b7cf
>> --- /dev/null
>> +++ b/security/timgad/timgad_lsm.c
>> @@ -0,0 +1,327 @@
>> +/*
>> + * Timgad Linux Security Module
>> + *
>> + * Author: Djalal Harouni
>> + *
>> + * Copyright (C) 2017 Djalal Harouni
>> + * Copyright (C) 2017 Endocode AG.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/lsm_hooks.h>
>> +#include <linux/sysctl.h>
>> +#include <linux/prctl.h>
>> +#include <linux/types.h>
>> +
>> +#include "timgad_core.h"
>> +
>> +static int module_restrict;
>> +
>> +static int zero;
>> +static int max_module_restrict_scope = TIMGAD_MODULE_NO_LOAD;
>> +
>> +/* TODO:
>> + *  complete permission check
>> + *  inline function logic with the per-process if possible
>> + */
>> +static int timgad_has_global_sysctl_perm(unsigned long op)
>> +{
>> +       int ret = -EINVAL;
>> +       struct mm_struct *mm = NULL;
>> +
>> +       if (op != PR_TIMGAD_GET_MOD_RESTRICT)
>> +               return ret;
>> +
>> +       switch (module_restrict) {
>> +       case TIMGAD_MODULE_OFF:
>> +               ret = 0;
>> +               break;
>> +       /* TODO: complete this and handle it later per task too */
>> +       case TIMGAD_MODULE_STRICT:
>> +               /*
>> +                * Are we allowed to sleep here ?
>> +                * Also improve this check here
>> +                */
>> +               ret = -EPERM;
>> +               mm = get_task_mm(current);
>> +               if (mm) {
>> +                       if (capable(CAP_SYS_MODULE))
>> +                               ret = 0;
>> +                       mmput(mm);
>> +               }
>
> Why the mm check here?

Due to lack of time, I could not check this so I just put this here in
case we have drivers pushed through a work job from a kthread... So to
not forget about it. I'm not sure about this, I will revisit it of
course.


>> +               break;
>> +       case TIMGAD_MODULE_NO_LOAD:
>> +               ret = -EPERM;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/* TODO: simplify me and move me to timgad_core.c file */
>> +static int module_timgad_task_perm(struct timgad_task *timgad_tsk,
>> +                                  char *kmod_name)
>> +{
>> +       int ret;
>> +       unsigned long flag = 0;
>> +
>> +       ret = is_timgad_task_op_set(timgad_tsk,
>> +                                   PR_TIMGAD_SET_MOD_RESTRICT, &flag);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       /*
>> +        * TODO: complete me
>> +        *    * Allow net modules only with CAP_NET_ADMIN and other cases...
>> +        *    * Other exotic cases when set to STRICT should be denied...
>> +        *    * Inline logic
>> +        */
>> +       switch (flag) {
>> +       case TIMGAD_MODULE_OFF:
>> +               ret = 0;
>> +               break;
>> +       case TIMGAD_MODULE_STRICT:
>> +               if (!capable(CAP_SYS_MODULE))
>> +                       ret = -EPERM;
>> +               else
>> +                       ret = 0;
>> +               break;
>> +       case TIMGAD_MODULE_NO_LOAD:
>> +               ret = -EPERM;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/* Set the given option in a timgad task */
>> +static int timgad_set_op_value(struct task_struct *tsk,
>> +                              unsigned long op, unsigned long value)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *ttask;
>> +       unsigned long flag = 0;
>> +
>> +       ret = timgad_op_to_flag(op, value, &flag);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ttask = get_timgad_task(tsk);
>> +       if (!ttask) {
>> +               ttask = give_me_timgad_task(tsk, value);
>> +               if (IS_ERR(ttask))
>> +                       return PTR_ERR(ttask);
>> +
>> +               return 0;
>> +       }
>> +
>> +       ret = timgad_task_set_op_flag(ttask, op, flag, value);
>> +
>> +       put_timgad_task(ttask, NULL);
>> +       return ret;
>> +}
>> +
>> +/* Get the given option from a timgad task */
>> +static int timgad_get_op_value(struct task_struct *tsk, unsigned long op)
>> +{
>> +       int ret = -EINVAL;
>> +       struct timgad_task *ttask;
>> +       unsigned long flag = 0;
>> +
>> +       ttask = get_timgad_task(tsk);
>> +       if (!ttask)
>> +               return ret;
>> +
>> +       ret = is_timgad_task_op_set(ttask, op, &flag);
>> +       put_timgad_task(ttask, NULL);
>> +
>> +       return ret < 0 ? ret : flag;
>> +}
>> +
>> +/* Copy Timgad context from parent to child */
>> +int timgad_task_copy(struct task_struct *tsk)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *tparent;
>> +       struct timgad_task *ttask;
>> +       unsigned long value = 0;
>> +
>> +       tparent = get_timgad_task(current);
>> +
>> +       /* Parent does not have a timgad context, nothing to do */
>> +       if (tparent == NULL)
>> +               return 0;
>> +
>> +       value = read_timgad_task_flags(tparent);
>> +
>> +       ttask = give_me_timgad_task(tsk, value);
>> +       if (IS_ERR(ttask))
>> +               ret = PTR_ERR(ttask);
>> +       else
>> +               ret = 0;
>> +
>> +       put_timgad_task(tparent, NULL);
>> +       return ret;
>> +}
>> +
>> +/*
>> + * Return 0 on success, -error on error.  -EINVAL is returned when Timgad
>> + * does not handle the given option.
>> + */
>> +int timgad_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>> +                     unsigned long arg4, unsigned long arg5)
>> +{
>> +       int ret = -EINVAL;
>> +       struct task_struct *myself = current;
>> +
>> +       if (option != PR_TIMGAD_OPTS)
>> +               return 0;
>> +
>
> Actually enforce that arg4 and arg5 == 0 here, and -EINVAL if they
> don't. This will let you expand in the future if you need to.

Ok, thanks will do.

>
>> +       get_task_struct(myself);
>> +
>> +       switch (arg2) {
>> +       case PR_TIMGAD_SET_MOD_RESTRICT:
>> +               ret = timgad_set_op_value(myself,
>> +                                         PR_TIMGAD_SET_MOD_RESTRICT,
>> +                                         arg3);
>> +               break;
>> +       case PR_TIMGAD_GET_MOD_RESTRICT:
>> +               ret = timgad_get_op_value(myself,
>> +                                         PR_TIMGAD_SET_MOD_RESTRICT);
>> +               break;
>> +       }
>> +
>> +       put_task_struct(myself);
>> +       return ret;
>> +}
>> +
>> +/*
>> + * Free the specific task attached resources
>> + * task_free() can be called from interrupt context
>> + */
>> +void timgad_task_free(struct task_struct *tsk)
>> +{
>> +       release_timgad_task(tsk);
>> +}
>> +
>> +static int timgad_kernel_module_file(struct file *file)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *ttask;
>> +       struct task_struct *myself = current;
>> +
>> +       /* First check if the task allows that */
>> +       ttask = get_timgad_task(myself);
>> +       if (ttask != NULL) {
>> +               ret = module_timgad_task_perm(ttask, NULL);
>> +               put_timgad_task(ttask, NULL);
>> +       }
>> +
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
>> +}
>
> This is the kernel_module_file hook, which is an explicit load of a
> module by the current process. Is your intention to block
> CAP_SYS_MODULE-capable processes from direct kernel module loading? If
> so, I misunderstood part of the design here. I'd explicitly call it
> out in the commit text, since direct loading and auto loading are
> quite different, though I'd argue that you don't want to touch direct
> loading privileges because that's already entirely covered by
> CAP_SYS_MODULE....

Makes sense, I'll update and confirm that we don't need this in case
we don't have context info about modprobe caller, otherwise we may
need this to fill the gap using a global sysctl policy, or maybe have
semantics based on module types and aliases... for auto-load, the
higher value will deny all auto-load operations.


>
>> +static int timgad_kernel_module_request(char *kmod_name)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *ttask;
>> +       struct task_struct *myself = current;
>> +
>> +       /* First check if the task allows that */
>> +       ttask = get_timgad_task(myself);
>> +       if (ttask != NULL) {
>> +               ret = module_timgad_task_perm(ttask, kmod_name);
>> +               put_timgad_task(ttask, NULL);
>> +       }
>> +
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
>> +}
>
> This is the heart of the auto-loading check...
>
>> +static int timgad_kernel_read_file(struct file *file,
>> +                                  enum kernel_read_file_id id)
>> +{
>> +       int ret = 0;
>> +
>> +       switch (id) {
>> +       case READING_MODULE:
>> +               ret = timgad_kernel_module_file(file);
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static struct security_hook_list timgad_hooks[] = {
>> +       LSM_HOOK_INIT(kernel_module_request, timgad_kernel_module_request),
>> +       LSM_HOOK_INIT(kernel_read_file, timgad_kernel_read_file),
>> +       LSM_HOOK_INIT(task_copy, timgad_task_copy),
>> +       LSM_HOOK_INIT(task_prctl, timgad_task_prctl),
>> +       LSM_HOOK_INIT(task_free, timgad_task_free),
>> +};
>> +
>> +#ifdef CONFIG_SYSCTL
>> +static int timgad_mod_dointvec_minmax(struct ctl_table *table, int write,
>> +                                     void __user *buffer, size_t *lenp,
>> +                                     loff_t *ppos)
>> +{
>> +       struct ctl_table table_copy;
>> +
>> +       if (write && !capable(CAP_SYS_MODULE))
>> +               return -EPERM;
>> +
>> +       table_copy = *table;
>> +       if (*(int *)table_copy.data == *(int *)table_copy.extra2)
>> +               table_copy.extra1 = table_copy.extra2;
>
> Are you intentionally pinning to the highest setting (like Yama)? This
> isn't mentioned in the commit message, and it doesn't seem like
> something that matters here?


Yes, I forget to document that. Thanks!

>
>> +
>> +       return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>> +}
>> +
>> +struct ctl_path timgad_sysctl_path[] = {
>> +       { .procname = "kernel", },
>> +       { .procname = "timgad", },
>> +       { }
>> +};
>> +
>> +static struct ctl_table timgad_sysctl_table[] = {
>> +       {
>> +               .procname       = "module_restrict",
>> +               .data           = &module_restrict,
>> +               .maxlen         = sizeof(int),
>> +               .mode           = 0644,
>> +               .proc_handler   = timgad_mod_dointvec_minmax,
>> +               .extra1         = &zero,
>> +               .extra2         = &max_module_restrict_scope,
>> +       },
>> +       { }
>> +};
>> +
>> +static void __init timgad_init_sysctl(void)
>> +{
>> +       if (!register_sysctl_paths(timgad_sysctl_path, timgad_sysctl_table))
>> +               panic("Timgad: sysctl registration failed.\n");
>> +}
>> +#else
>> +static inline void timgad_init_sysctl(void) { }
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +void __init timgad_add_hooks(void)
>> +{
>> +       pr_info("Timgad: becoming mindful.\n");
>
> That's Yama's catch-phrase. ;) Your LSM should say something else. :)

oups, hehe I hope I can find something!

>
>> +       security_add_hooks(timgad_hooks, ARRAY_SIZE(timgad_hooks));
>> +       timgad_init_sysctl();
>> +
>> +       if (timgad_tasks_init())
>> +               panic("Timgad: tasks initialization failed.\n");
>> +}
>> --
>> 2.5.5
>>
>
> Sorry for the delay in review! It's been a busy week. :)
>
> Thanks!

Thanks for the review Kees, I will revisit all this, fix based on
suggestions and update after next merge window. In mean time I'll
follow also Casey's stacking effort.

> -Kees
>
> --
> Kees Cook
> Pixel Security



-- 
tixxdz