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: <CA+55aFzPnO7L9nszpcxwQJ35Qq-35XCpGZneZvLFmJEZUJfHLQ@mail.gmail.com>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ