Re: [PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux Security Module

[Djalal Harouni]

On Mon, Apr 10, 2017 at 5:42 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
[...]
>> +
>> +static inline struct modautoload_task *init_modautoload_task(struct task_struct *tsk,
>> +                                                          unsigned long flags)
>> +{
>> +     struct modautoload_task *modtask;
>> +
>> +     modtask = task_security(tsk, modautorestrict_task_security_index);
>> +
>> +     modtask->flags = (u8)flags;
>
> I don't think you want to do this cast.

Will fix it. Thanks!


[...]
>> +
>> +#ifdef CONFIG_SYSCTL
>> +static int modautoload_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)
>
> While it's probably doing what you want, I find this
> sort of casting disturbing.

Ok will try to improve it.


>> +             table_copy.extra1 = table_copy.extra2;
>> +
>> +     return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>> +}
>> +
>> +struct ctl_path modautoload_sysctl_path[] = {
>> +     { .procname = "kernel", },
>> +     { .procname = "modautorestrict", },
>> +     { }
>> +};
>> +
>> +static struct ctl_table modautoload_sysctl_table[] = {
>> +     {
>> +             .procname       = "autoload",
>> +             .data           = &autoload_restrict,
>> +             .maxlen         = sizeof(int),
>> +             .mode           = 0644,
>> +             .proc_handler   = modautoload_dointvec_minmax,
>> +             .extra1         = &zero,
>> +             .extra2         = &max_autoload_restrict,
>> +     },
>> +     { }
>> +};
>> +
>> +static void __init modautoload_init_sysctl(void)
>> +{
>> +     if (!register_sysctl_paths(modautoload_sysctl_path, modautoload_sysctl_table))
>> +             panic("modautorestrict: sysctl registration failed.\n");
>> +}
>> +#else
>> +static inline void modautoload_init_sysctl(void) { }
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +void __init modautorestrict_init(void)
>> +{
>> +     modautorestrict_task_security_index =
>> +             security_reserve_task_blob_index(sizeof(struct modautoload_task));
>> +     security_add_hooks(modautoload_hooks,
>> +                        ARRAY_SIZE(modautoload_hooks), "modautorestrict");
>> +
>> +     modautoload_init_sysctl();
>> +     pr_info("ModAutoRestrict LSM:  Initialized\n");
>> +}
>> diff --git a/security/security.c b/security/security.c
>> index 4dc6bca..d8852fe 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -70,6 +70,7 @@ int __init security_init(void)
>>       capability_add_hooks();
>>       yama_add_hooks();
>>       loadpin_add_hooks();
>> +     modautorestrict_init();
>
> This should be modautorestrict_add_hooks() if this were
> a "minor" module, but as it's using a blob it is a "major"
> module. Either way, this is not right.

Do you mean that if I'm using a blob, it should go with the rest LSMs
in do_security_initcalls() ?

>>
>>       /*
>>        * Load all the remaining security modules.
>

Thanks for the comments!


-- 
tixxdz