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

[Linus Torvalds]

On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <keescook@xxxxxxxxxxxx> 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