Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 10 Apr 2017 20:27:55 +0200
From: Djalal Harouni <tixxdz@...il.com>
To: Casey Schaufler <casey@...aufler-ca.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Andy Lutomirski <luto@...nel.org>, 
	Kees Cook <keescook@...omium.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	kernel-hardening@...ts.openwall.com, 
	LSM List <linux-security-module@...r.kernel.org>, 
	Linux API <linux-api@...r.kernel.org>, Dongsu Park <dpark@...teo.net>, 
	James Morris <james.l.morris@...cle.com>, "Serge E. Hallyn" <serge@...lyn.com>, 
	Paul Moore <paul@...l-moore.com>, Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux
 Security Module

On Mon, Apr 10, 2017 at 5:42 PM, Casey Schaufler <casey@...aufler-ca.com> 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

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.