From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBotQhnHT2Dtun9uFbumfsG7SfUrsnvyvE7DVnw8i1u1JTZEnB6nNfbd0v5J+VFzRn/CAEX2z ARC-Seal: i=1; a=rsa-sha256; t=1516384137; cv=none; d=google.com; s=arc-20160816; b=Mh39r8CJ6ocZHNq/CCnbVLIRm5Sf3aP+ZjYjYcEuDwUmUNlqxRCAt+5ZHcenoKgSuP a2tZ256w6MR6NS/7VeUtle3GA46ju52yjEOs++gSzBs20GlbSupJzo9OrwhLUJg900mI iWG9GDe1AGICV0scUri3OIEN/I5fTapUC69HxzoJkJtxgZXp4iFnJ0osym6t7shRZY2Q vnyxymky143ojmXPYlqJFWXxNdNQtW9yZSfqJ4VS8eIrJXfxIyOI9GPqKboplhr3FC3R o+IwbaVijcA7LFMHQWkkMEvtBwdxXOnr2d66glgtzTX5/8lV9D8yIAtnN5bKPOi352NA V1MA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:user-agent:message-id:in-reply-to:date:references :organization:subject:cc:to:from:arc-authentication-results; bh=rBlTOz1PHGI5LpNteDVMYuR7NoEKncRE6hWh40QxPyw=; b=tJrJlNdDcdjPIm6XDq+xCBi5UBTSBYhEMBdDW61U/3pTyh97niunfpz1q2JhsgMdW9 vwaAQu3dKkftsyrbdUNQoZpDug06N1bMd4ymsd9Px1YDJJ/lpygjJpD2plabbaTXgQM4 zmBIvBoKx1sKCy+y1R4w7DPG5j4AVLJvIwPbWkxqGlKmS9lpnV17PUctmU6PLAA60WsV 0NGe4Q/Nu8biFoWHEEBrVWba7wHUtoKXdK3ihOS2phDlA0SvBNm9ivJXwSv4qDA99QUG 1N4G6rR2q0fDgoODKp5TBNzz+iPjN5YUFy2pf9QzHxt+M+bVYdi+XitJDWIQA5jXoW6d Puyw== ARC-Authentication-Results: i=1; mx.google.com; spf=neutral (google.com: 81.187.30.51 is neither permitted nor denied by best guess record for domain of ats@offog.org) smtp.mailfrom=ats@offog.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=offog.org Authentication-Results: mx.google.com; spf=neutral (google.com: 81.187.30.51 is neither permitted nor denied by best guess record for domain of ats@offog.org) smtp.mailfrom=ats@offog.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=offog.org From: Adam Sampson To: Jann Horn Cc: Dan Williams , kernel list , linux-arch , Kernel Hardening , Catalin Marinas , "the arch\/x86 maintainers" , Will Deacon , Russell King , Ingo Molnar , Greg Kroah-Hartman , "H. Peter Anvin" , Thomas Gleixner , Linus Torvalds , Andrew Morton , alan@linux.intel.com Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references Organization: I'll send this message down the wire and hope that someone wise is listening References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632010687.21271.12004432287640499992.stgit@dwillia2-desk3.amr.corp.intel.com> Date: Fri, 19 Jan 2018 17:48:04 +0000 In-Reply-To: (Jann Horn's message of "Fri, 19 Jan 2018 11:20:48 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1589977446378237360?= X-GMAIL-MSGID: =?utf-8?q?1590044013771475556?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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? Thanks, -- Adam Sampson From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adam Sampson Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references Date: Fri, 19 Jan 2018 17:48:04 +0000 Message-ID: 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 Return-path: In-Reply-To: (Jann Horn's message of "Fri, 19 Jan 2018 11:20:48 +0100") Sender: linux-kernel-owner@vger.kernel.org To: Jann Horn Cc: Dan Williams , kernel list , linux-arch , Kernel Hardening , Catalin Marinas , the arch/x86 maintainers , Will Deacon , Russell King , Ingo Molnar , Greg Kroah-Hartman , "H. Peter Anvin" , Thomas Gleixner , Linus Torvalds , Andrew Morton , alan@linux.intel.com List-Id: linux-arch.vger.kernel.org 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? Thanks, -- Adam Sampson From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from a-painless.mh.aa.net.uk ([81.187.30.51]:56129 "EHLO a-painless.mh.aa.net.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755949AbeASSRM (ORCPT ); Fri, 19 Jan 2018 13:17:12 -0500 From: Adam Sampson Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632010687.21271.12004432287640499992.stgit@dwillia2-desk3.amr.corp.intel.com> Date: Fri, 19 Jan 2018 17:48:04 +0000 In-Reply-To: (Jann Horn's message of "Fri, 19 Jan 2018 11:20:48 +0100") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Sender: linux-arch-owner@vger.kernel.org List-ID: To: Jann Horn Cc: Dan Williams , kernel list , linux-arch , Kernel Hardening , Catalin Marinas , the arch/x86 maintainers , Will Deacon , Russell King , Ingo Molnar , Greg Kroah-Hartman , "H. Peter Anvin" , Thomas Gleixner , Linus Torvalds , Andrew Morton , alan@linux.intel.com Message-ID: <20180119174804.ngq0Y3Er2lE5OY0421QntnGpkQmg-sSlnAPbZEP0bIA@z> 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 mboxrd@z Thu Jan 1 00:00:00 1970 From: Adam Sampson References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632010687.21271.12004432287640499992.stgit@dwillia2-desk3.amr.corp.intel.com> Date: Fri, 19 Jan 2018 17:48:04 +0000 In-Reply-To: (Jann Horn's message of "Fri, 19 Jan 2018 11:20:48 +0100") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [kernel-hardening] [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references To: Jann Horn Cc: Dan Williams , kernel list , linux-arch , Kernel Hardening , Catalin Marinas , the arch/x86 maintainers , Will Deacon , Russell King , Ingo Molnar , Greg Kroah-Hartman , "H. Peter Anvin" , Thomas Gleixner , Linus Torvalds , Andrew Morton , alan@linux.intel.com List-ID: 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? Thanks, -- Adam Sampson