On 7 September 2016 at 19:17, Linus Torvalds
<[email protected]> wrote:
> On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <[email protected]> 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
