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

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+55aFy82Drgu2xm1GWaDz2Jyj5QBx3dW4AatXzmCx-b1YET5Q@mail.gmail.com>
Date: Fri, 9 Mar 2018 13:10:30 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Kees Cook <keescook@...omium.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Josh Poimboeuf <jpoimboe@...hat.com>, 
	Rasmus Villemoes <linux@...musvillemoes.dk>, "Gustavo A. R. Silva" <gustavo@...eddedor.com>, 
	"Tobin C. Harding" <me@...in.cc>, Steven Rostedt <rostedt@...dmis.org>, Jonathan Corbet <corbet@....net>, 
	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>, David Sterba <dsterba@...e.com>, 
	"David S. Miller" <davem@...emloft.net>, Alexey Kuznetsov <kuznet@....inr.ac.ru>, 
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>, Ingo Molnar <mingo@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Masahiro Yamada <yamada.masahiro@...ionext.com>, Borislav Petkov <bp@...e.de>, 
	Randy Dunlap <rdunlap@...radead.org>, Ian Abbott <abbotti@....co.uk>, 
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>, Petr Mladek <pmladek@...e.com>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Pantelis Antoniou <pantelis.antoniou@...sulko.com>, Linux Btrfs <linux-btrfs@...r.kernel.org>, 
	Network Development <netdev@...r.kernel.org>, 
	Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()

On Fri, Mar 9, 2018 at 12:05 PM, Kees Cook <keescook@...omium.org> 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
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.