[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Jan 2018 23:09:54 -0800
From: Cyril Novikov <cnovikov@...x.com>
To: Dan Williams <dan.j.williams@...el.com>, linux-kernel@...r.kernel.org
Cc: linux-arch@...r.kernel.org, kernel-hardening@...ts.openwall.com,
Catalin Marinas <catalin.marinas@....com>, x86@...nel.org,
Will Deacon <will.deacon@....com>, Russell King <linux@...linux.org.uk>,
Ingo Molnar <mingo@...hat.com>, gregkh@...uxfoundation.org,
"H. Peter Anvin" <hpa@...or.com>, tglx@...utronix.de,
torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
alan@...ux.intel.com
Subject: Re: [PATCH v4 02/10] asm/nospec, array_ptr: sanitize speculative
array de-references
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
