From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: ARC-Seal: i=1; a=rsa-sha256; t=1517129704; cv=none; d=google.com; s=arc-20160816; b=RnsoDU0QAB9V2vhkR/JWojlCeLRrB1c0SmDfhaPwaQpckYB0CAHzjiaXlTeEu9O25h PNiGOLnQLjyR022Wy4v/N9I4TsIYIVTTZcchbxJC0sy+e1JVZ2SBf55x8d9EF1DnUh7U XU6LHlFliX43Ojh3wCGHr5bNMl8yAD+OyI7sVLdXVGTBeFDFr43kFWYHzFNV1Nr0MCwF NZKlAmE9mNvs7adhFa20qopqSeKU2W0HmE0RCuqYBk5edir4Jh3bWtTX1s0ps0BFI4f4 w9ugXhOcl2bi2OYDf15mHNwmifOnbiPj1Kkn/4qcma0Gyp99e53BRUA68blFOpxH+sD7 uArw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:sender:dkim-signature :arc-authentication-results; bh=ycKIVEniEP3PaJBPHIY5Uz30uV9L8uTp3vDp7AGuOa8=; b=wLoCuWaONMHAzql75hM4aw+zoHwrw/FLtNstc6YWcXHPuuAU7cdn/biC/wDXdHFPAE AWioB4+WNU9UrhXrLbxYJ+UWNfHFugMb8FdUV4p+pVYfgCvUIXDIENIOJLFz05vM4A50 67dzOMfcMb4krT4JDNnmBv3vOzCqL+MWlji9SVhjlVyBMgIArOsfYpFyBApFwGyqtrSw 314KsdU53bdFXd1TWnSxGYo8uVeQMnlNGU+rCgdVHZZy8Ux6mJ8tWwkhtLhRXypUqW4o E84OITOyaDD9pJ2bME6C8vkpGa5CSqLXAnz0n+AidXLSNA/h8Db/u5Q2mEF+QE4N52/N 24Sw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Qgjcy8PX; spf=pass (google.com: domain of mingo.kernel.org@gmail.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=mingo.kernel.org@gmail.com Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Qgjcy8PX; spf=pass (google.com: domain of mingo.kernel.org@gmail.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=mingo.kernel.org@gmail.com X-Google-Smtp-Source: AH8x2246MBDWpQbOnRZ6Qpw8T7WnzpI8QmclWf75cc2fzp8VDenGcj5VtMwXCuL6GQQqc+eURf5W1A== Sender: Ingo Molnar 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) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1590732017653801778?= X-GMAIL-MSGID: =?utf-8?q?1590825796518840999?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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