Re: [PATCH v2 06/19] asm-generic/barrier: mask speculative execution flows

[Dan Williams]

On Fri, Jan 12, 2018 at 1:12 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Thu, Jan 11, 2018 at 04:46:56PM -0800, Dan Williams wrote:
>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>> new file mode 100644
>> index 000000000000..5c66fc30f919
>> --- /dev/null
>> +++ b/include/linux/nospec.h
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright(c) 2018 Intel Corporation. All rights reserved.
>> +
>> +#ifndef __NOSPEC_H__
>> +#define __NOSPEC_H__
>> +
>> +#include <linux/jump_label.h>
>> +#include <asm/barrier.h>
>> +
>> +#ifndef array_ptr_mask
>> +#define array_ptr_mask(idx, sz)                                              \
>> +({                                                                   \
>> +     unsigned long mask;                                             \
>> +     unsigned long _i = (idx);                                       \
>> +     unsigned long _s = (sz);                                        \
>> +                                                                     \
>> +     mask = ~(long)(_i | (_s - 1 - _i)) >> (BITS_PER_LONG - 1);      \
>> +     mask;                                                           \
>> +})
>> +#endif
>> +
>> +/**
>> + * __array_ptr - Generate a pointer to an array element, ensuring
>> + * the pointer is bounded under speculation to NULL.
>> + *
>> + * @base: the base of the array
>> + * @idx: the index of the element, must be less than LONG_MAX
>> + * @sz: the number of elements in the array, must be less than LONG_MAX
>> + *
>> + * If @idx falls in the interval [0, @sz), returns the pointer to
>> + * @arr[@idx], otherwise returns NULL.
>> + */
>> +#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;                                                       \
>> +})
>> +
>> +#ifdef CONFIG_SPECTRE1_IFENCE
>> +DECLARE_STATIC_KEY_TRUE(nospec_key);
>> +#else
>> +DECLARE_STATIC_KEY_FALSE(nospec_key);
>> +#endif
>> +
>> +#ifdef ifence_array_ptr
>> +/*
>> + * The expectation is that no compiler or cpu will mishandle __array_ptr
>> + * leading to problematic speculative execution. Bypass the ifence
>> + * based implementation by default.
>> + */
>> +#define array_ptr(base, idx, sz)                             \
>> +({                                                           \
>> +     typeof(*(base)) *__ret;                                 \
>> +                                                             \
>> +     if (static_branch_unlikely(&nospec_key))                \
>> +             __ret = ifence_array_ptr(base, idx, sz);        \
>> +     else                                                    \
>> +             __ret = __array_ptr(base, idx, sz);             \
>> +     __ret;                                                  \
>> +})
>
>
> So I think this wants:
>
> #ifndef HAVE_JUMP_LABEL
> #error Compiler lacks asm-goto, can generate unsafe code
> #endif
>
> Suppose the generic array_ptr_mask() is unsafe on some arch and they
> only implement ifence_array_ptr() and they compile without asm-goto,
> then the above reverts to a dynamic condition, which can be speculated.
> If we then speculate into the 'bad' __array_ptr we're screwed.

True.

>
>> +#else
>> +#define array_ptr __array_ptr
>> +#endif
>> +
>> +#endif /* __NOSPEC_H__ */
>
>
> In general I think I would write all this in a form like:
>
> #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;                                                       \
> })
>
> #if defined(array_ptr_mask) && defined(ifence_array_ptr)
>
> #ifndef HAVE_JUMP_LABEL
> #error Compiler lacks asm-goto, can generate unsafe code
> #endif
>
> #define array_ptr(base, idx, sz)                                \
> ({                                                              \
>         typeof(*(base)) *__ret;                                 \
>                                                                 \
>         if (static_branch_unlikely(&nospec_key))                \
>                 __ret = ifence_array_ptr(base, idx, sz);        \
>         else                                                    \
>                 __ret = __array_ptr(base, idx, sz);             \
>         __ret;                                                  \
> })
>
> #elif defined(array_ptr_mask)
>
> #define array_ptr(base, idx, sz) __array_ptr(base, idx, sz)
>
> #elif defined(ifence_array_ptr)
>
> #define array_ptr(base, idx, sz) ifence_array_ptr(base, idx, sz)
>
> #else
>
> /* XXX we want a suitable warning here ? */
>
> #define array_ptr(base, idx, sz) (idx < sz ? base + idx : NULL)
>
> #endif
>
> and stick the generic array_ptr_mask into asm-generic/nospec.h or
> something.
>
> Then the static key stuff is limited to architectures that define _both_
> array_ptr_mask and ifence_array_ptr.

This certainly needs a Kconfig "depends on JUMP_LABEL" to turn on the
dynamic switching at all, and a HAVE_JUMP_LABEL compile time failure
if the compiler lacks support. I don't think we need the checks on
'defined(array_ptr_mask) or that 'XXX' warning case, because default
mask is assumed safe, and otherwise better than nothing.