Received: by 10.223.176.5 with SMTP id f5csp1432272wra; Sun, 28 Jan 2018 00:55:44 -0800 (PST) X-Google-Smtp-Source: AH8x227dQ7jbOQ+aYOiF9YAhTsOfIuskkYuDvs5ntV4OATSGe0yjsLRUYebVZQdhABD6QOKLtAej X-Received: by 10.98.57.27 with SMTP id g27mr24010596pfa.128.1517129744525; Sun, 28 Jan 2018 00:55:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517129744; cv=none; d=google.com; s=arc-20160816; b=a4nIwy9esIxavyel2wSLY1aS6d/hx+5S5O50JJ1eaYP+Qo5Yj52DIPg9MVxqye4Zll BDw8AV+AQurncuDsbvf007uUh/0HFLqYAoQoQH2Di9iM+5ykXtbW1+9QwTaxButB4XRD ATicVshzRRgE9aw4qayXnfrY1vwIJbr8730pZeQ0UDPbY7OAhZf+9E5l0WSHCGt52kgq H6H4uEFPHe5SWxV0JfA8kT3EOLTkKYZR4pBY35cFx/jIZVxd+rGWTvKnTKtse3nVBOj1 C7ao1lB5kuA19IUD7Ce6AS/sGJVkCLooK6Obkp6TfHAcVFHfCmkBuBkSHiDSQ++CsXKh Gb4g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature:arc-authentication-results; bh=ycKIVEniEP3PaJBPHIY5Uz30uV9L8uTp3vDp7AGuOa8=; b=TPm3FgZ7c6wWCc8LJV7CpUIyvaFXhUjbyW5OeW4fJBaoI9NVNZfBCh4rlbf0Pqal7Y luSu7JOueoeYHtiRf67vpOIrt4aCohFmyvgwTWCPzeKS9Agh+oo0gg00GTVJKNfmdBEd STy1Z5WIiTt7cW6ptHshK2XnrQzm0eOOXJxqq1lS35bJwAcCbW91/1er9Nmv2uHLhi9B 2TklxzNnAC9NtdzugyXz08eKf4MUGdW7n8Oi9WhRYs7Q5ihzCw6rD/Q2M1sBBFPoWmz0 w0dO2bhlcDWUWugI/wxWzj+4IRQVxAJMvSPX0FFi7oTm7hHGfdiou8O95/flI5PgNS0g VD5Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=Qgjcy8PX; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id u10-v6si6634288plu.505.2018.01.28.00.55.29; Sun, 28 Jan 2018 00:55:44 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=Qgjcy8PX; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751296AbeA1IzG (ORCPT + 99 others); Sun, 28 Jan 2018 03:55:06 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:55924 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750819AbeA1IzF (ORCPT ); Sun, 28 Jan 2018 03:55:05 -0500 Received: by mail-wm0-f65.google.com with SMTP id 143so8518771wma.5; Sun, 28 Jan 2018 00:55:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ycKIVEniEP3PaJBPHIY5Uz30uV9L8uTp3vDp7AGuOa8=; b=Qgjcy8PXvHheFhsOrpq1KjDTR2kczzLBbqvsdg1N1MwMJfbVnh3+37bCaYlTsL/9H2 CRooz5PBzyHGAoELT9OBK0XKPNyVmFUVDldZpg9ilLxKvGiKVhGdqniAI7sfK1NkMaMt U2DSRhYBIZsek/JjqNzWB5Znu+XnUKWErBjofGJjQW5brDPzmVbKQIfg0ZDoHvemlQDw Bllh80FGYVwJdZ6S7kbZdGggmS83krp6YVPFsofuE2L+nHiS7KZxhHh7yZfZTBQRR7+T Ektp7HLLiLWUxtrVmhWipPd7DLO/3cQOxXc87P0f1yoZxbOoZib6ZPngURsZgLLCn66J 0E2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=ycKIVEniEP3PaJBPHIY5Uz30uV9L8uTp3vDp7AGuOa8=; b=VC1DETrTS7OZv7KOY2WTZRTEm8iFJgJwXZeCw9Gdnvp/Ff0Sx8s0QMokSbNO5yJ0w4 tNBYMBN/IbF8EmdK3HlC7QG2COzPivwB34u82ItJKTUy12bMe0jbOepE9HFjsCfS/jgJ PER3Ks77cDMGxea4rFiGReZo9Bv/N3WfGY5PdOgbcA3bUryqAoBCZd3UNoFU/ZTwx1Ib qe76JmAUdABAUgU+q69EZCf4KHnvsUzc14tCk6eFC/ZWAQ6U76ZyxSYPDebS/4m7n8i+ 7f5FQg8bXQnqkqZPL6fDByuqGGqmlvwFBzelK3Dx3KVnnJIUyFz82uVZoWRSn3niNM9P Trcw== X-Gm-Message-State: AKwxytcZzTEgoNezCOyB0xcdI/u3JnrCYMby7xe97Gv+KdlkjE/L0rsL Led6uYACX9tNe6EYhFm9cK8= X-Received: by 10.28.241.4 with SMTP id p4mr13792400wmh.103.1517129703525; Sun, 28 Jan 2018 00:55:03 -0800 (PST) Received: from gmail.com (2E8B0CD5.catv.pool.telekom.hu. [46.139.12.213]) by smtp.gmail.com with ESMTPSA id v9sm8905507wre.8.2018.01.28.00.55.01 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 28 Jan 2018 00:55:02 -0800 (PST) Date: Sun, 28 Jan 2018 09:55:00 +0100 From: Ingo Molnar To: Dan Williams Cc: tglx@linutronix.de, linux-arch@vger.kernel.org, Cyril Novikov , kernel-hardening@lists.openwall.com, Peter Zijlstra , Catalin Marinas , x86@kernel.org, Will Deacon , Russell King , Ingo Molnar , gregkh@linuxfoundation.org, "H. Peter Anvin" , torvalds@linux-foundation.org, alan@linux.intel.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 02/12] array_idx: sanitize speculative array de-references Message-ID: <20180128085500.djlm5rlbhjlpfj4i@gmail.com> References: <151703971300.26578.1185595719337719486.stgit@dwillia2-desk3.amr.corp.intel.com> <151703972396.26578.7326612698912543866.stgit@dwillia2-desk3.amr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <151703972396.26578.7326612698912543866.stgit@dwillia2-desk3.amr.corp.intel.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Firstly, I only got a few patches of this series so I couldn't review all of them - please Cc: me to all future Meltdown and Spectre related patches! * Dan Williams wrote: > 'array_idx' is proposed as a generic mechanism to mitigate against > Spectre-variant-1 attacks, i.e. an attack that bypasses boundary checks > via speculative execution). The 'array_idx' implementation is expected > to be safe for current generation cpus across multiple architectures > (ARM, x86). nit: Stray closing parenthesis s/cpus/CPUs > Based on an original implementation by Linus Torvalds, tweaked to remove > speculative flows by Alexei Starovoitov, and tweaked again by Linus to > introduce an x86 assembly implementation for the mask generation. > > Co-developed-by: Linus Torvalds > Co-developed-by: Alexei Starovoitov > Suggested-by: Cyril Novikov > Cc: Russell King > Cc: Peter Zijlstra > Cc: Catalin Marinas > Cc: Will Deacon > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: x86@kernel.org > Signed-off-by: Dan Williams > --- > include/linux/nospec.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > create mode 100644 include/linux/nospec.h > > diff --git a/include/linux/nospec.h b/include/linux/nospec.h > new file mode 100644 > index 000000000000..f59f81889ba3 > --- /dev/null > +++ b/include/linux/nospec.h > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright(c) 2018 Intel Corporation. All rights reserved. Given the close similarity of Linus's array_access() prototype pseudocode there should probably also be: Copyright (C) 2018 Linus Torvalds in that file? > + > +#ifndef __NOSPEC_H__ > +#define __NOSPEC_H__ > + > +/* > + * When idx is out of bounds (idx >= sz), the sign bit will be set. > + * Extend the sign bit to all bits and invert, giving a result of zero > + * for an out of bounds idx, or ~0UL if within bounds [0, sz). > + */ > +#ifndef array_idx_mask > +static inline unsigned long array_idx_mask(unsigned long idx, unsigned long sz) > +{ > + /* > + * Warn developers about inappropriate array_idx usage. > + * > + * Even if the cpu speculates past the WARN_ONCE branch, the s/cpu/CPU > + * sign bit of idx is taken into account when generating the > + * mask. > + * > + * This warning is compiled out when the compiler can infer that > + * idx and sz are less than LONG_MAX. Please use 'idx' and 'sz' in quotes, to make sure they stand out more in free flowing comment text. Also please use '()' to denote functions/methods. I.e. something like: * Warn developers about inappropriate array_idx() usage. * * Even if the CPU speculates past the WARN_ONCE() branch, the * sign bit of 'idx' is taken into account when generating the * mask. * * This warning is compiled out when the compiler can infer that * 'idx' and 'sz' are less than LONG_MAX. That's just one example - please apply it to all comments consistently. > + */ > + if (WARN_ONCE(idx > LONG_MAX || sz > LONG_MAX, > + "array_idx limited to range of [0, LONG_MAX]\n")) Same in user facing messages: "array_idx() limited to range of [0, LONG_MAX]\n")) > + * For a code sequence like: > + * > + * if (idx < sz) { > + * idx = array_idx(idx, sz); > + * val = array[idx]; > + * } > + * > + * ...if the cpu speculates past the bounds check then array_idx() will > + * clamp the index within the range of [0, sz). s/cpu/CPU > + */ > +#define array_idx(idx, sz) \ > +({ \ > + typeof(idx) _i = (idx); \ > + typeof(sz) _s = (sz); \ > + unsigned long _mask = array_idx_mask(_i, _s); \ > + \ > + BUILD_BUG_ON(sizeof(_i) > sizeof(long)); \ > + BUILD_BUG_ON(sizeof(_s) > sizeof(long)); \ > + \ > + _i &= _mask; \ > + _i; \ > +}) > +#endif /* __NOSPEC_H__ */ For heaven's sake, please name a size variable as 'size', not 'sz'. We don't have a shortage of characters and can deobfuscate common primitives, can we? Also, beyond the nits, I also hate the namespace here. We have a new generic header providing two new methods: #include array_idx_mask() array_idx() which is then optimized for x86 in asm/barrier.h. That's already a non-sequitor. Then we introduce uaccess API variants with a _nospec() postfix. Then we add ifence() to x86. There's no naming coherency to this. A better approach would be to signal the 'no speculation' aspect of the array_idx() methods already: naming it array_idx_nospec() would be a solution, as it clearly avoids speculation beyond the array boundaries. Also, without seeing the full series it's hard to tell, whether the introduction of linux/nospec.h is justified, but it feels somewhat suspect. Thanks, Ingo