Received: by 10.223.176.5 with SMTP id f5csp1797289wra; Sun, 28 Jan 2018 08:29:13 -0800 (PST) X-Google-Smtp-Source: AH8x226g8pVS+PdR0ltD189WYHcO/65XpOl7VIqp3FkgVEz0grNeyTYLp+8oNz6F8U4ejx2YDA99 X-Received: by 10.99.123.91 with SMTP id k27mr19715896pgn.179.1517156953377; Sun, 28 Jan 2018 08:29:13 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517156953; cv=none; d=google.com; s=arc-20160816; b=v3VYlhttTep8wyQt0ILJLgm5B192kwU9YjIgvF+b2NjFCx5mOXdGOUiZ2lZbcGU7aP BVP7LAcJqyJ6g7Q1bGUL0WbLGoOzmpuYBt/uyqPG568SS9ase+GE4by3qQQDVCI6QDoF qI/OdugkripCkdrRDWcmca9fot6tao+gi6xbVa8UMos+mD5iMg8vQhikClVA+UunrnCR 9R9Yga63CYqTGrWTulX1ZPvV7Y2raHauWQiU1i54TC3TBdETBdoqTsDzuuvbCGdcdCeQ ZsJeCIY81Ku91jdcZHHyIBrexfvpx1qhleTYIjwDpEpXfaU26ZK3g0krcqoUT/TBp3E2 JUhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature :arc-authentication-results; bh=Pesyos6hhpPm8hyzo/6kplPIctv8TcNIMaB4h98EPok=; b=lO8RqbuxRNj2/AXiuhBga5LqyhLDpgD+CPUxEms4Vb7RlwWp0InAGt8ld8hj7HwIyn HvP4ClqKDMPho+hPsKYHVcknnqQqyrRK5sI7/m/++wt+H48QuZwjUUPo1I3JpBnzw6zi nIWKYBPFqaM47Bh9l+KDjbPbk9Z2h1rF6jCYrvVcfhauTQIEL8qI2upwrhkMUn+Do7vZ uD/1+1gyQewhwknHk1uBPk08e6Nay1mzm0mAXd5SyGlC4AOEq5uVRxLL+fGGlEu0GzGV fR1w+qQZZx5VRZm2Nate+k795gSPz2m+Yt4b1mt+P1D4Gp2/v4g1oBhUWMQgk4pL9rFb VG1w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel-com.20150623.gappssmtp.com header.s=20150623 header.b=KyXwYwZW; 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 x3-v6si7426399plv.589.2018.01.28.08.28.59; Sun, 28 Jan 2018 08:29:13 -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=pass header.i=@intel-com.20150623.gappssmtp.com header.s=20150623 header.b=KyXwYwZW; 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 S1752028AbeA1Q21 (ORCPT + 99 others); Sun, 28 Jan 2018 11:28:27 -0500 Received: from mail-oi0-f68.google.com ([209.85.218.68]:33011 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751674AbeA1Q2Z (ORCPT ); Sun, 28 Jan 2018 11:28:25 -0500 Received: by mail-oi0-f68.google.com with SMTP id l8so3420831oig.0 for ; Sun, 28 Jan 2018 08:28:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Pesyos6hhpPm8hyzo/6kplPIctv8TcNIMaB4h98EPok=; b=KyXwYwZW0j22ZOA6z04hJieLzKFXc4xHyn+VL0dN52CrGdnuYwFABZFHrFLkMZmeg9 wbfrPLFKLb7DG7mzELlaAbqMIVsZnJESbj2+Yq1vRkXUpN6QXnX/8EJBmmlI3Ojuk3ef 8IRgoHmAFaJ+9kjdsGrE+Io2A94TE8j5MDKQoltqry03+l7Fdmmsk9CPSkowc51D7Mje GhEiupV2nuJDIYfetrQahjopVs9uoQ0Oiw17NU7Hhgz5gCJChhYZz8BOMcNu9tD4wLIp 8vG/p2YZmHHWy5OngPiCeZRJNJE27rplmxVct+Uv7td0uMpwVao5bnZZP7Q0b3waJ2cJ LTJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Pesyos6hhpPm8hyzo/6kplPIctv8TcNIMaB4h98EPok=; b=umtp9TZfQhA/1RSTv5nXlDeKU7t40Uq5U4c0s/Eq+nhJy8zDeI8j722KwvMUYSd213 AW7I/s7UvW/Ok5a9sBE8hzlvdZ8ndy+X8X22J/Fkebb6fMepe3dv5thr147O/Xy25TKG p2BIi6yyIWqad1yU8pgxBrX6eJ7L6qhehOl7TKyYPX4g+oVw/4oIa3RG/dGk1CFxxzyv zA4Isox8cLH+BGwziAze0Ym23BUkfvIW/KwiM4A+/ruWdMRxBtHGJozlXiJ9Bo+Rd/TX yY4ejUoz2KZmY2d3T/nssEdiw1WxJ/7RjgZOPijRFxB6F0QdWAzJxDuZ2LSGdPgp0kJ/ gp2A== X-Gm-Message-State: AKwxytdDKSgogTIH6DtW6gzCQ2y8E/cfmg4w6G+2mxMT+TMwuGU4twjs FUWmNRw3i/0JY/U6HE6dLO/SKrT0zN2oi+9ya6xIUw== X-Received: by 10.202.51.135 with SMTP id z129mr17192135oiz.175.1517156904771; Sun, 28 Jan 2018 08:28:24 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.62.91 with HTTP; Sun, 28 Jan 2018 08:28:24 -0800 (PST) In-Reply-To: <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> <20180128085500.djlm5rlbhjlpfj4i@gmail.com> From: Dan Williams Date: Sun, 28 Jan 2018 08:28:24 -0800 Message-ID: Subject: Re: [PATCH v5 02/12] array_idx: sanitize speculative array de-references To: Ingo Molnar Cc: Thomas Gleixner , linux-arch , Cyril Novikov , Kernel Hardening , Peter Zijlstra , Catalin Marinas , X86 ML , Will Deacon , Russell King , Ingo Molnar , Greg KH , "H. Peter Anvin" , Linus Torvalds , Alan Cox , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jan 28, 2018 at 12:55 AM, Ingo Molnar wrote: > > 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? Yes, and Alexei as well. > >> + >> +#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. Ingo, I love you, but please take the incredulity down a bit, especially when I had 'nospec' in all the names in v1. Thomas, Peter, and Alexei wanted s/nospec_barrier/ifence/ and s/array_idx_nospec/array_idx/. You can always follow on with a patch to fix up the names and placements to your liking. While they'll pick on my name choices, they won't pick on yours, because I simply can't be bothered to care about a bikeshed color at this point after being bounced around for 5 revisions of this patch set. > 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. >