From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x225hMKigIxXAk2gNBD7NF+Ob9TOjQ5yMy8F/A2eCcgxwomEu4X5dBmSGNMXWWHGgwbLfizkq ARC-Seal: i=1; a=rsa-sha256; t=1516864166; cv=none; d=google.com; s=arc-20160816; b=fWT2rzXXzFZHXsDGbVD3NcxGzYGaCf8xjZU2PqzvuBZ7VJueRZHKvsBlrGv/ZheTnb C8UX5J29St/qE3X5s6UKLZvbLMVw6vZyesm5gCAjFY9mLrxSO0eE001zrTAOYtv1zCy8 P7zQjbf9zeAT4fW4D0xFP6yFfdqKd0Qcbvj1B/2KKOfRhYfM2TLvHW3xIgjk7U4pCJQG mPzxPYxERMtN4omuLXFjpyrCLZ3adzB/3T6erI8T4QT4rKvSGZmpDl8P9hKIqHZjYy3l rWpzCs+9JTuOCrJxynma20wNsUOyHAu9YSGLFmoqYgdVhFYMcdvRIThZZwfg30vuynaz m2RQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :arc-authentication-results; bh=jnfLWigF1eZPyH+ouc2f1buH2ziWh7vqYgfWnRZjtY0=; b=0QKpA9OC8Uye4CU409QpPsNBauvf+CEOiPTXe5v8HQsVGCh5ZNeVWATYjBzXlHgZ03 7IRiORVm/Ak6Xvac7zNSEuE+7hiYnxPTGKwVltoeMRxlaJbAITQFKjPvDiL4zY6RBg12 ustGatJBqJNwJIVSH988v8cFpjvDneyaf/NuTB3mi5HyIXFCSeDStHAC+qGxClgKSsK+ QCSeTufFQjXWNN6yxZZjyNUMee1FEqRVuyiFx8USZSHEY2Hw3+aj3fE4ia9kowplleTk FDNSd7Png0Kf5aYqCey6D/L8DbI4lsxBIiSeHiLijoJB0HRav61AQKo3JooTNkdeD7GV BfwQ== ARC-Authentication-Results: i=1; mx.google.com; spf=neutral (google.com: 184.106.54.75 is neither permitted nor denied by best guess record for domain of cnovikov@lynx.com) smtp.mailfrom=cnovikov@lynx.com Authentication-Results: mx.google.com; spf=neutral (google.com: 184.106.54.75 is neither permitted nor denied by best guess record for domain of cnovikov@lynx.com) smtp.mailfrom=cnovikov@lynx.com X-Auth-ID: cnovikov@lynx.com X-Sender-Id: cnovikov@lynx.com Subject: Re: [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative array de-references 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 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 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 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 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1589977446378237360?= X-GMAIL-MSGID: =?utf-8?q?1590547360458314464?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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