From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references Date: Fri, 19 Jan 2018 18:18:33 +0000 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 Return-path: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Content-Disposition: inline In-Reply-To: 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 List-Id: linux-arch.vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com ([217.140.101.70]:41790 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755949AbeASSS1 (ORCPT ); Fri, 19 Jan 2018 13:18:27 -0500 Date: Fri, 19 Jan 2018 18:18:33 +0000 From: Will Deacon 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: Sender: linux-arch-owner@vger.kernel.org List-ID: 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 Message-ID: <20180119181833.qnFQiD2l7_WrCHjtJlmS9B4aq17fM-jsTNbG_5C6ARw@z> 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