From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBouvqirGB7IDSf0GUf9q8b/OHUmMl8opsYG2iIbbLAcqd+zXMgOAGkWXCXPo3LIlrjZa5L+H ARC-Seal: i=1; a=rsa-sha256; t=1516385907; cv=none; d=google.com; s=arc-20160816; b=fipZGbeRsqabPAHiUA0NfjQ/ANHIhPGh3EBe001EXeGb+t3E7I7fX4j9A3XEL65enf ZqtXNlTYlHH/QbaK1+KkTFeRCEdtnMnc6KRYtZQyAbHrxlsl2ru9hihvde+W6oK84utu /Q9eKftQyhIz49uOXPTNNFAF4vtgD4HrMQF55pmiPhXVxkVC5NUD1Iz61qEQnZqt/CWb kU+0hwgBfqo+EzBR3uvlyCh58p+hoJnAz3F+kkirhquHVPOLpnv279h/NmYBmFxxngWr DI2i1HNIcJHu3hCEsw78FCax4uCZyIXrueFXhDHtrYMY5O3gASzzz9nLFsPDrjyScEEh si9A== 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:arc-authentication-results; bh=4XhHurMK/2nXtYrxVRiwjVry8OK39pOlUSg5N+xy4l8=; b=DifyWwxLf2hl7iv9TylToPrJSeyOzrxdk8VSu6EoXCyrRDuKSfbmsInoIdXjOsTzBA 4nfdwJDXo/4FyugKYeXePtFGLYWNhLKaR4c/N0RRV2Zcl5oPLuVO2JbqDht2xXGe8ZiA +CJxPQ4V8Tic6NkBiLg/bYYx/rGlrP3gQ2wM2z0AuTdgtFdsRUjGyzabSZbNlZotlF7l QuENfq6+bsz5GJtPMtj3Sx0sgSK60cGdLB64ZTdPUpFbS+xwThhMImmz7sBvrtJAvqrJ 4E5oZpQ/RppFbdBowhAkCXDXIYxsJvb6netIbckIcpN6YVyLYBllg5UlNhUYcKg3srbb mtsA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of will.deacon@arm.com designates 217.140.101.70 as permitted sender) smtp.mailfrom=will.deacon@arm.com Authentication-Results: mx.google.com; spf=pass (google.com: domain of will.deacon@arm.com designates 217.140.101.70 as permitted sender) smtp.mailfrom=will.deacon@arm.com Date: Fri, 19 Jan 2018 18:18:33 +0000 From: Will Deacon To: Dan Williams 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 Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1589977446378237360?= X-GMAIL-MSGID: =?utf-8?q?1590045869803463698?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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. Will