From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from Galois.linutronix.de ([146.0.238.70]:38535 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751107AbeCHLWR (ORCPT ); Thu, 8 Mar 2018 06:22:17 -0500 Date: Thu, 8 Mar 2018 12:21:23 +0100 (CET) From: Thomas Gleixner To: Rasmus Villemoes cc: Kees Cook , Andrew Morton , linux-kernel@vger.kernel.org, corbet@lwn.net, gustavo@embeddedor.com, rostedt@goodmis.org, Chris Mason , Josef Bacik , David Sterba , "David S. Miller" , Alexey Kuznetsov , Hideaki YOSHIFUJI , Ingo Molnar , Peter Zijlstra , Masahiro Yamada , Borislav Petkov , Josh Poimboeuf , Randy Dunlap , Ian Abbott , "Tobin C. Harding" , Sergey Senozhatsky , Petr Mladek , Andy Shevchenko , Pantelis Antoniou , linux-btrfs@vger.kernel.org, netdev@vger.kernel.org, kernel-hardening@lists.openwall.com Subject: Re: [PATCH v2 1/3] vsprintf: Remove accidental VLA usage In-Reply-To: <49e6b509-d1e7-5916-ecb8-6bde026fde1e@rasmusvillemoes.dk> Message-ID: References: <1520479847-39174-1-git-send-email-keescook@chromium.org> <1520479847-39174-2-git-send-email-keescook@chromium.org> <49e6b509-d1e7-5916-ecb8-6bde026fde1e@rasmusvillemoes.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-btrfs-owner@vger.kernel.org List-ID: On Thu, 8 Mar 2018, Rasmus Villemoes wrote: > On 2018-03-08 04:30, Kees Cook wrote: > > /** > > + * SIMPLE_MAX - return maximum of two values without any type checking > > + * @x: first value > > + * @y: second value > > + * > > + * This should only be used in stack array sizes, since the type-checking > > + * from max() confuses the compiler into thinking a VLA is being used. > > + */ > > +#define SIMPLE_MAX(x, y) ((size_t)(x) > (size_t)(y) ? (size_t)(x) \ > > + : (size_t)(y)) > > This will be abused at some point, leading to the usual double > evaluation etc. etc. problems. The name is also too long (and in general > we should avoid adjectives like "simple", "safe", people reading the > code won't know what is simple or safe about it). I think this should work > > #define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y)) > > That forces (x)>(y) to be a compile-time constant, so x and y must also > be; hence there can be no side effects. The MIN version of this could > replace the custom __const_min in fs/file.c, and probably other places > as well. > > I tested that this at least works in the vsprintf case, -Wvla no longer > complains. fs/file.c also compiles with the MIN version of this. > > I suppose MIN and MAX will collide with other uses in the tree. Hmm. Make it CONST_MAX() or something like that which makes it entirely clear. Thanks, tglx From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Gleixner Subject: Re: [PATCH v2 1/3] vsprintf: Remove accidental VLA usage Date: Thu, 8 Mar 2018 12:21:23 +0100 (CET) Message-ID: References: <1520479847-39174-1-git-send-email-keescook@chromium.org> <1520479847-39174-2-git-send-email-keescook@chromium.org> <49e6b509-d1e7-5916-ecb8-6bde026fde1e@rasmusvillemoes.dk> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Cc: Kees Cook , Andrew Morton , linux-kernel@vger.kernel.org, corbet@lwn.net, gustavo@embeddedor.com, rostedt@goodmis.org, Chris Mason , Josef Bacik , David Sterba , "David S. Miller" , Alexey Kuznetsov , Hideaki YOSHIFUJI , Ingo Molnar , Peter Zijlstra , Masahiro Yamada , Borislav Petkov , Josh Poimboeuf , Randy Dunlap , Ian Abbott , "Tobin C. Harding" , Sergey Senozhatsky , Petr Mladek Return-path: In-Reply-To: <49e6b509-d1e7-5916-ecb8-6bde026fde1e@rasmusvillemoes.dk> Sender: linux-btrfs-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Thu, 8 Mar 2018, Rasmus Villemoes wrote: > On 2018-03-08 04:30, Kees Cook wrote: > > /** > > + * SIMPLE_MAX - return maximum of two values without any type checking > > + * @x: first value > > + * @y: second value > > + * > > + * This should only be used in stack array sizes, since the type-checking > > + * from max() confuses the compiler into thinking a VLA is being used. > > + */ > > +#define SIMPLE_MAX(x, y) ((size_t)(x) > (size_t)(y) ? (size_t)(x) \ > > + : (size_t)(y)) > > This will be abused at some point, leading to the usual double > evaluation etc. etc. problems. The name is also too long (and in general > we should avoid adjectives like "simple", "safe", people reading the > code won't know what is simple or safe about it). I think this should work > > #define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y)) > > That forces (x)>(y) to be a compile-time constant, so x and y must also > be; hence there can be no side effects. The MIN version of this could > replace the custom __const_min in fs/file.c, and probably other places > as well. > > I tested that this at least works in the vsprintf case, -Wvla no longer > complains. fs/file.c also compiles with the MIN version of this. > > I suppose MIN and MAX will collide with other uses in the tree. Hmm. Make it CONST_MAX() or something like that which makes it entirely clear. Thanks, tglx