Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

From: Kees Cook
Date: Fri Mar 09 2018 - 16:47:17 EST

Next message: Jaegeuk Kim: "Re: [PATCH] f2fs: avoid selinux denial on CAP_SYS_RESOURCE"
Previous message: Stephen Boyd: "Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs"
In reply to: Linus Torvalds: "Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()"
Next in thread: Tobin C. Harding: "Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Mar 9, 2018 at 12:05 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> When max() is used in stack array size calculations from literal values
>> (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
>> thinks this is a dynamic calculation due to the single-eval logic, which
>> is not needed in the literal case. This change removes several accidental
>> stack VLAs from an x86 allmodconfig build:
>
> Ok, looks good.
>
> I just have a couple of questions about applying it.
>
> In particular, if this will help people working on getting rid of
> VLA's in the short term, I can apply it directly.

AFAIK, all the VLAs effected by max() are fixed with this patch. i.e.
no other VLA work I'm aware of depends on this (famous last words).

> But if people who are looking at it (anybody else than Kees?)

FWIW, I've seen at least 6 other people helping out now with VLA clean
up patches. Whee. :)

> don't much care, then this
> might be a 4.17 thing or at least "random -mm queue"?

Andrew has this (v2) queued in -mm for 4.17. I didn't view VLA clean
up (or this macro change) as urgent by any means.

> #define __single_eval_max(max1, max2, x, y) ({ \
> typeof (x) max1 = (x); \
> typeof (y) max2 = (y); \
> max1 > max2 ? max1 : max2; })
>
> actually looks more natural to me than passing the two types in as
> arguments to the macro.

I (obviously) didn't design this macro originally, but my take on this
was that it's safer to keep the type check together with the
comparison so that it is never possible for someone to run off and use
__single_eval_max() directly and accidentally bypass the type check.
While it does look silly from the max_t()/min_t() perspective, it just
seems more defensive.

> (That form also is amenable to things like "__auto_type" etc simplifications).

Agreed, I think that's the biggest reason to lift the types. However,
since we're still not actually doing anything with the types (i.e.
this change doesn't weaken the type checking), I would think it would
be orthogonal. While it would be nice to resolve differing types
sanely, there doesn't appear to be a driving reason to make this
change. The example case of max(5, sizeof(thing)) doesn't currently
exist in the code and I don't see how making min()/max() _less_ strict
would be generically useful (in fact, it has proven useful to have
strict type checking).

> Side note: do we *really* need the unique variable names? That's what
> makes those things _really_ illegible. I thgink it's done just for a
> sparse warning that we should probably ignore. It's off by default
> anyway exactly because it doesn't work that well due to nested macro
> expansions like this.

That I'm not sure about. I'd be fine to remove them; I left them in
place because it seemed quite intentional. :)

> There is very real value to keeping our odd macros legible, I feel.
> Even when they are complicated by issues like this, it would be good
> to keep them as simple as possible.
>
> Comments?

I'm open to whatever! I'm just glad this gets to kill a handful of
"accidental" stack VLAs. :)

-Kees

--
Kees Cook
Pixel Security<div class="gmail_extra"> <div class="gmail_quote">On
Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds <<a
href="mailto:torvalds@xxxxxxxxxxxxxxxxxxxx";
target="_blank">torvalds@xxxxxxxxxxxxxxxxxxxx</a>>
wrote: <blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">On
Fri, Mar 9, 2018 at 12:05 PM, Kees Cook <<a
href="mailto:keescook@xxxxxxxxxxxx";>keescook@xxxxxxxxxxxx</a>>
wrote: 
> When max() is used in stack array size calculations from literal values 
> (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler 
> thinks this is a dynamic calculation due to the single-eval
logic, which 
> is not needed in the literal case. This change removes several
accidental 
> stack VLAs from an x86 allmodconfig build: 
 
Ok, looks good. 
 
I just have a couple of questions about applying it. 
 
In particular, if this will help people working on getting rid of 
VLA's in the short term, I can apply it directly. But if people who 
are looking at it (anybody else than Kees?) don't much care, then this 
might be a 4.17 thing or at least "random -mm queue"? 
 
The other unrelated reaction I had to this was that "we're passing 
those types down very deep, even though nobody _cares_ about them all 
that much at that deep level". 
 
Honestly, the only case that really cares is the very top level, and 
the rest could just take the properly cast versions. 
 
For example, "max_t/min_t" really don't care at all, since they - by 
definition - just take the single specified type. 
 
So I'm wondering if we should just drop the types from __max/__min 
(and everything they call) entirely, and instead do 
 
    #define __check_type(x,y)
((void)((typeof(x)*)1==(typeof(y)*)1)) 
    #define min(x,y)   (__check_type(x,y),__min(x,y)) 
    #define max(x,y)   (__check_type(x,y),__max(x,y)) 
 
    #define min_t(t,x,y) __min((t)(x),(t)(y)) 
    #define max_t(t,x,y) __max((t)(x),(t)(y)) 
 
and then __min/__max and friends are much simpler (and can just assume 
that the type is already fine, and the casting has been done). 
 
This is technically entirely independent of this VLA cleanup thing, 
but the "passing the types around unnecessarily" just becomes more 
obvious when there's now another level of macros, _and_ it's a more 
complex expression too. 
 
Yes, yes, the __single_eval_xyz() functions still end up wanting the 
types for the declaration of the new single-evaluation variables, but 
the 'typeof' pattern is the standard pattern, so 
 
#define __single_eval_max(max1, max2, x, y) ({  \ 
        typeof (x) max1 = (x);     
            \ 
        typeof (y) max2 = (y);     
            \ 
        max1 > max2 ? max1 : max2; }) 
 
actually looks more natural to me than passing the two types in as 
arguments to the macro. 
 
(That form also is amenable to things like "__auto_type" etc
simplifications). 
 
Side note: do we *really* need the unique variable names? That's what 
makes those things _really_ illegible. I thgink it's done just for a 
sparse warning that we should probably ignore. It's off by default 
anyway exactly because it doesn't work that well due to nested macro 
expansions like this. 
 
There is very real value to keeping our odd macros legible, I feel. 
Even when they are complicated by issues like this, it would be good 
to keep them as simple as possible. 
 
Comments? 
 
                Linus 
</blockquote></div> <div> </div>--
 <div class="gmail_signature" data-smartmail="gmail_signature">Kees
Cook Pixel Security</div>
</div>

Next message: Jaegeuk Kim: "Re: [PATCH] f2fs: avoid selinux denial on CAP_SYS_RESOURCE"
Previous message: Stephen Boyd: "Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs"
In reply to: Linus Torvalds: "Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()"
Next in thread: Tobin C. Harding: "Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]