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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEiveUeYB8jQoP9R9xSj+5dxc3cfCg46Z=C5TkfkYibDWrEbPQ@mail.gmail.com>
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