linux-kernel - Re: [PATCH] usercopy: remove page-spanning test for now

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] [day] [month] [year] [list]

Message-ID: <CAOMGZ=EYDOWhTpx0KXNi6jJu+EPyY4AH7JkkeGAZYFL_6Bj2tg@mail.gmail.com>
Date:   Wed, 7 Sep 2016 22:32:42 +0200
From:   Vegard Nossum <vegard.nossum@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Rik van Riel <riel@...hat.com>,
        Laura Abbott <labbott@...hat.com>,
        VMware Graphics <linux-graphics-maintainer@...are.com>,
        Sinclair Yeh <syeh@...are.com>,
        Vinson Lee <vlee@...edesktop.org>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Andy Lutomirski <luto@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usercopy: remove page-spanning test for now

On 7 September 2016 at 19:17, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <keescook@...omium.org> wrote:
>>
>> +#ifndef CONFIG_HARDENED_USERCOPY_PAGESPAN
>> +       /*
>> +        * The page-spanning checks are hitting false positives, so
>> +        * do not check them for now.
>> +        */
>> +       return NULL;
>> +#endif
>> +
>>         /* Allow kernel data region (if not marked as Reserved). */
>>         if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
>>                 return NULL;
>
> No. Don't do patches like this.
>
> It's wrong for two reasons:
>
>  (a) if you want to use an #ifdef to disable code, do so. Enclose the
> code you want to disable with the #ifdef, not some *other* code that
> then indirectly disables the code you want to disable.
>
>  (b) don't do "surprising" things with control flow. It can cause
> compiler warnings in reasonable compilers ("unreachable code"), but
> it's also a strange pattern that throws people.
>
> So really, make the patch bigger but more legible. In fact, I think
> the best option would be to simply turn the code you want to disable
> into a helper function of its own, and then make the #ifdef enable or
> disable the whole function.
>
> (That also solves the problems like having the declaration for
> "endpage" and the other variables that is only used by the disabled
> code be *with* the disabled code, so that you don't have to have
> multiple ifdef'ed regions. I suspect avoiding that is a large reason
> why you did the hacky thing in the first place).

For this particular case, one might also use something like

if (!IS_ENABLED(CONFIG_HARDENED_USERCOPY_PAGESPAN))
    return;

no ifdefs, the "early return" is a common pattern (and readable IMHO),
and no unused variable warnings or separate ifdef blocks for variables
and I *think* no unreachable code warnings.

Or?
</bikeshed>


Vegard