From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cyril Novikov Subject: Re: [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references Date: Wed, 24 Jan 2018 23:09:54 -0800 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; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <151632010687.21271.12004432287640499992.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Dan Williams , linux-kernel@vger.kernel.org Cc: linux-arch@vger.kernel.org, kernel-hardening@lists.openwall.com, Catalin Marinas , x86@kernel.org, Will Deacon , Russell King , Ingo Molnar , gregkh@linuxfoundation.org, "H. Peter Anvin" , tglx@linutronix.de, torvalds@linux-foundation.org, akpm@linux-foundation.org, alan@linux.intel.com List-Id: linux-arch.vger.kernel.org On 1/18/2018 4:01 PM, Dan Williams wrote: > 'array_ptr' 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_ptr' implementation is expected > to be safe for current generation cpus across multiple architectures > (ARM, x86). I'm an outside reviewer, not subscribed to the list, so forgive me if I do something not according to protocol. I have the following comments on this change: After discarding the speculation barrier variant, is array_ptr() needed at all? You could have a simpler sanitizing macro, say #define array_sanitize_idx(idx, sz) ((idx) & array_ptr_mask((idx), (sz))) (adjusted to not evaluate idx twice). And use it as follows: if (idx < array_size) { idx = array_sanitize_idx(idx, array_size); do_something(array[idx]); } If I understand the speculation stuff correctly, unlike array_ptr(), this "leaks" array[0] rather than nothing (*NULL) when executed speculatively. However, it's still much better than leaking an arbitrary location in memory. The attacker can likely get array[0] "leaked" by passing 0 as idx anyway. > +/* > + * 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 Why does this have to resort to the undefined behavior of shifting a negative number to the right? You can do without it: return ((idx | (sz - 1 - idx)) >> (BITS_PER_LONG - 1)) - 1; Of course, you could argue that subtracting 1 from 0 to get all ones is also an undefined behavior, but it's still much better than the shift, isn't it? > +#define array_ptr(base, idx, sz) \ > +({ \ > + union { typeof(*(base)) *_ptr; unsigned long _bit; } __u; \ > + typeof(*(base)) *_arr = (base); \ > + unsigned long _i = (idx); \ > + unsigned long _mask = array_ptr_mask(_i, (sz)); \ > + \ > + __u._ptr = _arr + (_i & _mask); \ > + __u._bit &= _mask; \ > + __u._ptr; \ > +}) Call me paranoid, but I think this may actually create an exploitable bug on 32-bit systems due to casting the index to an unsigned long, if the index as it comes from userland is a 64-bit value. You have *replaced* the "if (idx < array_size)" check with checking if array_ptr() returns NULL. Well, it doesn't return NULL if the low 32 bits of the index are in-bounds, but the high 32 bits are not zero. Apart from the return value pointing to the wrong place, the subsequent code may then assume that the 64-bit idx is actually valid and trip on it badly. -- Cyril From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp64.ord1c.emailsrvr.com ([108.166.43.64]:59493 "EHLO smtp64.ord1c.emailsrvr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750763AbeAYHQl (ORCPT ); Thu, 25 Jan 2018 02:16:41 -0500 Received: from smtp1.relay.ord1c.emailsrvr.com (localhost [127.0.0.1]) by smtp1.relay.ord1c.emailsrvr.com (SMTP Server) with ESMTP id F41422013C for ; Thu, 25 Jan 2018 02:09:26 -0500 (EST) Received: from smtp72.ord1d.emailsrvr.com (relay.ord1c.rsapps.net [172.28.255.120]) by smtp1.relay.ord1c.emailsrvr.com (SMTP Server) with ESMTPS id EDBDD2013A for ; Thu, 25 Jan 2018 02:09:26 -0500 (EST) Subject: Re: [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> From: Cyril Novikov Message-ID: Date: Wed, 24 Jan 2018 23:09:54 -0800 MIME-Version: 1.0 In-Reply-To: <151632010687.21271.12004432287640499992.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dan Williams , linux-kernel@vger.kernel.org Cc: linux-arch@vger.kernel.org, kernel-hardening@lists.openwall.com, Catalin Marinas , x86@kernel.org, Will Deacon , Russell King , Ingo Molnar , gregkh@linuxfoundation.org, "H. Peter Anvin" , tglx@linutronix.de, torvalds@linux-foundation.org, akpm@linux-foundation.org, alan@linux.intel.com Message-ID: <20180125070954.1ctqJWYRA3i4_YzhhKNG8dV1PJjhY02Xi78xrg54q1k@z> On 1/18/2018 4:01 PM, Dan Williams wrote: > 'array_ptr' 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_ptr' implementation is expected > to be safe for current generation cpus across multiple architectures > (ARM, x86). I'm an outside reviewer, not subscribed to the list, so forgive me if I do something not according to protocol. I have the following comments on this change: After discarding the speculation barrier variant, is array_ptr() needed at all? You could have a simpler sanitizing macro, say #define array_sanitize_idx(idx, sz) ((idx) & array_ptr_mask((idx), (sz))) (adjusted to not evaluate idx twice). And use it as follows: if (idx < array_size) { idx = array_sanitize_idx(idx, array_size); do_something(array[idx]); } If I understand the speculation stuff correctly, unlike array_ptr(), this "leaks" array[0] rather than nothing (*NULL) when executed speculatively. However, it's still much better than leaking an arbitrary location in memory. The attacker can likely get array[0] "leaked" by passing 0 as idx anyway. > +/* > + * 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 Why does this have to resort to the undefined behavior of shifting a negative number to the right? You can do without it: return ((idx | (sz - 1 - idx)) >> (BITS_PER_LONG - 1)) - 1; Of course, you could argue that subtracting 1 from 0 to get all ones is also an undefined behavior, but it's still much better than the shift, isn't it? > +#define array_ptr(base, idx, sz) \ > +({ \ > + union { typeof(*(base)) *_ptr; unsigned long _bit; } __u; \ > + typeof(*(base)) *_arr = (base); \ > + unsigned long _i = (idx); \ > + unsigned long _mask = array_ptr_mask(_i, (sz)); \ > + \ > + __u._ptr = _arr + (_i & _mask); \ > + __u._bit &= _mask; \ > + __u._ptr; \ > +}) Call me paranoid, but I think this may actually create an exploitable bug on 32-bit systems due to casting the index to an unsigned long, if the index as it comes from userland is a 64-bit value. You have *replaced* the "if (idx < array_size)" check with checking if array_ptr() returns NULL. Well, it doesn't return NULL if the low 32 bits of the index are in-bounds, but the high 32 bits are not zero. Apart from the return value pointing to the wrong place, the subsequent code may then assume that the 64-bit idx is actually valid and trip on it badly. -- Cyril