From patchwork Fri Mar 16 19:27:23 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 10289901 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7863560386 for ; Fri, 16 Mar 2018 19:27:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 67ACA29016 for ; Fri, 16 Mar 2018 19:27:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5A4C729093; Fri, 16 Mar 2018 19:27:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id DC8B229016 for ; Fri, 16 Mar 2018 19:27:39 +0000 (UTC) Received: (qmail 23986 invoked by uid 550); 16 Mar 2018 19:27:37 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Received: (qmail 23963 invoked from network); 16 Mar 2018 19:27:36 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=5L9+zuxG+b36uQSq6IhNHxK41MqVBGzpS8ttiuArc1s=; b=LMRBOsE+PY5GZ1qFVuQawA8KpwGDV0lu2Hav9k4P7IeNcHsY2dMshnQP8fXiTQeTVE mqK7IRlwxgNEtBhmPle51JvL09YcHO4VT26HNnjXZrQlYaYAMQfA4C1h9L/nB/Tw25/W Qcu3tcjRQkIIINDghpE02od7apdJMXCOikKiLeHDR4TZ4IM9Yc705ToB5mU4mXa4+IAX Q8Xr8dbv8n4q8rq+uh1lupDyv+LFktBJ5xqJKA/JX6rBJWKzreG9T5o5F8++PFc4RiZt o1pwdvNLaIokCNWIx1mot9k5/UMHiTXnoOqolYVdGMMNj36qgMUIopmcKydETEeTdOKA 3Pnw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=5L9+zuxG+b36uQSq6IhNHxK41MqVBGzpS8ttiuArc1s=; b=TkZkM4yB6x0r9E7G5iz8oA2TVgW7i4PXffKU3p5NdFkSs0XfqJyWJ7xWcqJRyVenZf 9Dl+l2s4UfjMIJtj9UZO1F3kU2/I0o2j/fpxvgGNXzf28mbOdM06o0nXpd4NvFeFMqUl nlgB+IUu7TQAKxyJQW7E+k26BpvvCQk4oPNmk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=5L9+zuxG+b36uQSq6IhNHxK41MqVBGzpS8ttiuArc1s=; b=JL+RkjUfAEnea/5tbgq73xVwMg5QADO4+6rrGbGjp/koqk/XZZ7+oTj/rPB1xHAX+6 QTGmC3BMPLKByLZ1jjQD5cXw5Qp8BHV7Im0IpxkJ3R9L5RmhqnihVaPrMDStFbAUQ2FD XzKge9XqPy1I5Wn69tglTVSjDVquGHd2i+kcAkYMclVltZJTNWsEBOnPaIA9VnJw1J86 hywmNWX04hdmZxUgdEjra5UFzr8Qqt8HzM3H9AAQvZVxpgmjQvp+FgCxZtYFzjS9arZe /Mqa3s36sqhN/xgfSdZG9gFn7zEI5rD8lC6DuS87tLF5MC9x7KB3KZCnZdgYxs+QOMTI RmxQ== X-Gm-Message-State: AElRT7GKfHuGOxddHuRV5M5D1AhL4hD21VlIvIut0MmVverJmlwhniZX stY6QHMI1Z1UrqWYijqy6DUAsgvNCFnvdL5Bh40= X-Google-Smtp-Source: AG47ELvjQ0wB5gC1rbIcgzakFXHE9w8tS2Ap/DgaifJoTcvcmvogT2beBe1Q6I4Gbc9o7DhsUDsDx82KdnRM5KQA0Sc= X-Received: by 2002:a24:94cc:: with SMTP id j195-v6mr3487541ite.1.1521228444524; Fri, 16 Mar 2018 12:27:24 -0700 (PDT) MIME-Version: 1.0 Sender: linus971@gmail.com In-Reply-To: <20180316175502.GE30522@ZenIV.linux.org.uk> References: <1521174359-46392-1-git-send-email-keescook@chromium.org> <20180316175502.GE30522@ZenIV.linux.org.uk> From: Linus Torvalds Date: Fri, 16 Mar 2018 12:27:23 -0700 X-Google-Sender-Auth: Zy2xB92P7knWH7iwjXSS7fSrqDc Message-ID: Subject: Re: [PATCH v5 0/2] Remove false-positive VLAs when using max() To: Al Viro Cc: Florian Weimer , Kees Cook , Andrew Morton , Josh Poimboeuf , Rasmus Villemoes , Randy Dunlap , Miguel Ojeda , Ingo Molnar , David Laight , Ian Abbott , linux-input , linux-btrfs , Network Development , Linux Kernel Mailing List , Kernel Hardening X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Mar 16, 2018 at 10:55 AM, Al Viro wrote: > > That's not them, that's C standard regarding ICE. Yes. The C standard talks about "integer constant expression". I know. It's come up in this very thread before. The C standard at no point talks about - or forbids - "variable length arrays". That never comes up in the whole standard, I checked. So we are right now hindered by a _syntactic_ check, without any way to have a _semantic_ check. That's my problem. The warnings are misleading and imply semantics. And apparently there is no way to actually check semantics. > 1,100 is *not* a constant expression as far as the standard is concerned, I very much know. But it sure isn't "variable" either as far as the standard is concerned, because the standard doesn't even have that concept (it uses "variable" for argument numbers and for variables). So being pedantic doesn't really change anything. > Would you argue that in > void foo(char c) > { > int a[(c<<1) + 10 - c + 2 - c]; Yeah, I don't think that even counts as a constant value, even if it can be optimized to one. I would not at all be unhppy to see __builtin_constant_p() to return zero. But that is very much different from the syntax issue. So I would like to get some way to get both type-checking and constant checking without the annoying syntax issue. > expr, constant_expression is not a constant_expression. And in > this particular case the standard is not insane - the only reason > for using that is typechecking and _that_ can be achieved without > violating 6.6p6: > sizeof(expr,0) * 0 + ICE > *is* an integer constant expression, and it gives you exact same > typechecking. So if somebody wants to play odd games, they can > do that just fine, without complicating the logics for compilers... Now that actually looks like a good trick. Maybe we can use that instead of the comma expression that causes problems. And using sizeof() to make sure that __builtin_choose_expression() really gets an integer constant expression and that there should be no ambiguity looks good. Hmm. This works for me, and I'm being *very* careful (those casts to pointer types are inside that sizeof, because it's not an integral type, and non-integral casts are not valid in an ICE either) but somebody needs to check gcc-4.4: #define __typecheck(a,b) \ (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) #define __no_side_effects(a,b) \ (__builtin_constant_p(a)&&__builtin_constant_p(b)) #define __safe_cmp(a,b) \ (__typecheck(a,b) && __no_side_effects(a,b)) #define __cmp(a,b,op) ((a)op(b)?(a):(b)) #define __cmp_once(a,b,op) ({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ __cmp(__a,__b,op); }) #define __careful_cmp(a,b,op) \ __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), __cmp_once(a,b,op)) #define min(a,b) __careful_cmp(a,b,<) #define max(a,b) __careful_cmp(a,b,>) #define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<) #define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>) and yes, it does cause new warnings for that comparison between ‘enum tis_defaults’ and ‘enum tpm2_const’ in drivers/char/tpm/tpm_tis_core.h due to #define TIS_TIMEOUT_A_MAX max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) but technically that warning is actually correct, I'm just confused why gcc cares about the cast placement or something. That warning is easy to fix by turning it into a "max_t(int, enum1, enum2)' and that is technically the right thing to do, it's just not warned about for some odd reason with the current code. Kees - is there some online "gcc-4.4 checker" somewhere? This does seem to work with my gcc. I actually tested some of those files you pointed at now. Linus include/linux/kernel.h | 77 +++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 57 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 3fd291503576..23c31bf1d7fb 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -787,37 +787,29 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } * strict type-checking.. See the * "unnecessary" pointer comparison. */ -#define __min(t1, t2, min1, min2, x, y) ({ \ - t1 min1 = (x); \ - t2 min2 = (y); \ - (void) (&min1 == &min2); \ - min1 < min2 ? min1 : min2; }) +#define __typecheck(a,b) \ + (!!(sizeof((typeof(a)*)1==(typeof(b)*)1))) -/** - * min - return minimum of two values of the same or compatible types - * @x: first value - * @y: second value - */ -#define min(x, y) \ - __min(typeof(x), typeof(y), \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) +#define __no_side_effects(a,b) \ + (__builtin_constant_p(a)&&__builtin_constant_p(b)) -#define __max(t1, t2, max1, max2, x, y) ({ \ - t1 max1 = (x); \ - t2 max2 = (y); \ - (void) (&max1 == &max2); \ - max1 > max2 ? max1 : max2; }) +#define __safe_cmp(a,b) \ + (__typecheck(a,b) && __no_side_effects(a,b)) -/** - * max - return maximum of two values of the same or compatible types - * @x: first value - * @y: second value - */ -#define max(x, y) \ - __max(typeof(x), typeof(y), \ - __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \ - x, y) +#define __cmp(a,b,op) ((a)op(b)?(a):(b)) + +#define __cmp_once(a,b,op) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + __cmp(__a,__b,op); }) + +#define __careful_cmp(a,b,op) \ + __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), __cmp_once(a,b,op)) + +#define min(a,b) __careful_cmp(a,b,<) +#define max(a,b) __careful_cmp(a,b,>) +#define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<) +#define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>) /** * min3 - return minimum of three values @@ -856,35 +848,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } */ #define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi) -/* - * ..and if you can't take the strict - * types, you can specify one yourself. - * - * Or not use min/max/clamp at all, of course. - */ - -/** - * min_t - return minimum of two values, using the specified type - * @type: data type to use - * @x: first value - * @y: second value - */ -#define min_t(type, x, y) \ - __min(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) - -/** - * max_t - return maximum of two values, using the specified type - * @type: data type to use - * @x: first value - * @y: second value - */ -#define max_t(type, x, y) \ - __max(type, type, \ - __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \ - x, y) - /** * clamp_t - return a value clamped to a given range using a given type * @type: the type of variable to use