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

Date:   Wed, 7 Sep 2016 10:17:06 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     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 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).

                Linus