From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: ARC-Seal: i=1; a=rsa-sha256; t=1516386374; cv=none; d=google.com; s=arc-20160816; b=TPXGG563AJSTCn/M2nP/a6ju397z87qtX85uOLcun/0aCVcRb86gaNDA+/LrJARVT7 0GtWvyxLTG1CzoEi5XBtrUO/o2UWmvhnYFEQDXoJoA4IdFQTu1nHmhx555ADqZmIqKL0 n7pO/mOV6DiaWytKFoxwRkZFagPWRf1amPoKwp95Pz+TjP/Jisi+sMtZ02ykfU8KfwKt jKSMHAojrTmFkGLx75xLuwmBd+MpYbcnHDgKSjDG4XLi1DhARdhbuakOHisHnZwzLG2m 6+Cyd9nI6ywt8NvRwiewQiZusl81exmlJ5gl+afPsJs9OJo4JqIROisHcM5I2CQ03UyK nJnw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:dkim-signature:arc-authentication-results; bh=NbS8k5pPBxneeMdlPKhq/BYLDkkq8nfb4I+c8EzkOC8=; b=FMvw7j55M5NiH3cMFk5q7bTDlJmALiAjHy80POuvpXMv24d+iv0oCGXweuN94HL4M7 WRBL4ubimzKiShgF4RwLpxLViKJ2hq2AF88nr0ii5rJm5+saWf2mBPDoCin+QLSdWyZn aP3M2kDjEn9piU/VJsUXb3/C+zuPCEiAlM3PXdi7FeGPNVWPzmNrxjsqA+pA+TvCVozR 16f58XUFuoGQzWl30IS8iDO6RWZEAmKwcwj0L9hyPa4HTiFfo1EgRXEqcZ7qwSncFRTc Z1uAB7T90Vv1+Js1qbUFZv0kt4owWfxYtD0Mm1m7JKoeurNv/+4j1l8POIPSZa0Xs7Ea D6WA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel-com.20150623.gappssmtp.com header.s=20150623 header.b=r+Uezuio; spf=pass (google.com: domain of dan.j.williams@intel.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=dan.j.williams@intel.com Authentication-Results: mx.google.com; dkim=pass header.i=@intel-com.20150623.gappssmtp.com header.s=20150623 header.b=r+Uezuio; spf=pass (google.com: domain of dan.j.williams@intel.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=dan.j.williams@intel.com X-Google-Smtp-Source: ACJfBouSuSvRq4+qqX5QAC1AIX/OOqbLHUrg4oBrALd+RQ3SJx2D2SV2k+6HSGY51qaGLkr5meqgeXdenkp1dfd6q5Q= MIME-Version: 1.0 In-Reply-To: <20180119181833.GA1878@arm.com> References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632010687.21271.12004432287640499992.stgit@dwillia2-desk3.amr.corp.intel.com> <20180119181833.GA1878@arm.com> From: Dan Williams Date: Fri, 19 Jan 2018 10:26:13 -0800 Message-ID: Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references To: Will Deacon Cc: Adam Sampson , Jann Horn , kernel list , linux-arch , Kernel Hardening , Catalin Marinas , "the arch/x86 maintainers" , Russell King , Ingo Molnar , Greg Kroah-Hartman , "H. Peter Anvin" , Thomas Gleixner , Linus Torvalds , Andrew Morton , Alan Cox , Alexei Starovoitov Content-Type: text/plain; charset="UTF-8" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1589977446378237360?= X-GMAIL-MSGID: =?utf-8?q?1590046358949976017?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Fri, Jan 19, 2018 at 10:18 AM, Will Deacon wrote: > > On Fri, Jan 19, 2018 at 10:12:47AM -0800, Dan Williams wrote: > > [ adding Alexei back to the cc ] > > > > On Fri, Jan 19, 2018 at 9:48 AM, Adam Sampson wrote: > > > Jann Horn writes: > > > > > >>> +/* > > >>> + * If idx is negative or if idx > size then bit 63 is set in the mask, > > >>> + * and the value of ~(-1L) is zero. When the mask is zero, bounds check > > >>> + * failed, array_ptr will return NULL. > > >>> + */ > > >>> +#ifndef array_ptr_mask > > >>> +static inline unsigned long array_ptr_mask(unsigned long idx, > > >>> unsigned long sz) > > >>> +{ > > >>> + return ~(long)(idx | (sz - 1 - idx)) >> (BITS_PER_LONG - 1); > > >>> +} > > >>> +#endif > > >> > > >> Nit: Maybe add a comment saying that this is equivalent to > > >> "return ((long)idx >= 0 && idx < sz) ? ULONG_MAX : 0"? > > > > > > That's only true when sz < LONG_MAX, which is documented below but not > > > here; it's also different from the asm version, which doesn't do the idx > > > <= LONG_MAX check. So making the constraint explicit would be a good idea. > > > > > > From a bit of experimentation, when the top bit of sz is set, this > > > expression, the C version and the assembler version all have different > > > behaviour. For example, with 32-bit unsigned long: > > > > > > index=00000000 size=80000001: expr=ffffffff c=00000000 asm=ffffffff > > > index=80000000 size=80000001: expr=00000000 c=00000000 asm=ffffffff > > > index=00000000 size=a0000000: expr=ffffffff c=00000000 asm=ffffffff > > > index=00000001 size=a0000000: expr=ffffffff c=00000000 asm=ffffffff > > > index=fffffffe size=ffffffff: expr=00000000 c=00000000 asm=ffffffff > > > > > > It may be worth noting that: > > > > > > return 0 - ((long) (idx < sz)); > > > > > > causes GCC, on ia32 and amd64, to generate exactly the same cmp/sbb > > > sequence as in Linus's asm. Are there architectures where this form > > > would allow speculation? > > > > We're operating on the assumption that compilers will not try to > > introduce branches where they don't exist in the code, so if this is > > producing identical assembly I think we should go with it and drop the > > x86 array_ptr_mask. > > Branches, perhaps, but this could easily be compiled to a conditional > select (CSEL) instruction on arm64 and that wouldn't be safe without a > CSDB. Of course, we can do our own thing in assembly to prevent that, but > it would mean that the generic C implementation would not be robust for us. > Ah, ok good to know. Likely if the current version doesn't produce a conditional instruction on ARM perhaps it also won't do that on other architectures, so it is safer.