Re: [PATCH] kernel: sys: fix potential Spectre v1

[Dan Williams]

On Wed, May 23, 2018 at 8:07 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Wed, May 23, 2018 at 11:08:40AM +0200, Peter Zijlstra wrote:
>>
>> Sorry for being late to the party..
>
> Likewise!
>
>> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:
>> > +#define validate_index_nospec(index, size)                            \
>> > +({                                                                    \
>> > +       bool ret = true;                                               \
>> > +       typeof(index) *ptr = &(index);                                 \
>> > +       typeof(size) _s = (size);                                      \
>> > +                                                                      \
>> > +       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
>> > +       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
>> > +                                                                      \
>> > +       if (*ptr >= size)                                              \
>> > +               ret = false;                                           \
>> > +                                                                      \
>> > +       *ptr = array_index_nospec(*ptr, _s);                           \
>> > +                                                                      \
>> > +       ret;                                                           \
>> > +})
>>
>> Would not something like:
>>
>>       bool ret = false;
>>
>>       ....
>>
>>       if (*ptr < _s) {
>>               *ptr = array_index_nospec(*ptr, _s);
>>               ret = true;
>>       }
>>
>>       ret;
>>
>> be more obvious?
>
> I think that either way, we have a potential problem if the compiler
> generates a branch dependent on the result of validate_index_nospec().
>
> In that case, we could end up with codegen approximating:
>
>         bool safe = false;
>
>         if (idx < bound) {
>                 idx = array_index_nospec(idx, bound);
>                 safe = true;
>         }
>
>         // this branch can be mispredicted
>         if (safe) {
>                 foo = array[idx];
>         }
>
> ... and thus we lose the nospec protection.
>
> I also suspect that compiler transformations mean that this might
> already be the case for patterns like:
>
>         if (idx < bound)  {
>                 safe_idx = array_index_nospec(idx, bound)];
>                 ...
>                 foo = array[safe_idx];
>         }
>
> ... if the compiler can transform that to something like:
>
>         if (idx < bound) {
>                 idx = array_index_nospec(idx, bound);
>         }
>
>         // can be mispredicted
>         if (idx < bound) {
>                 foo = array[idx];
>         }
>
> ... which I think a compiler might be capable of, depending on the rest
> of the function body (e.g. if there's a common portion shared with the
> else case).
>
> I'll see if I can trigger that in a test case. :/

This would be interesting, because my operating assumption is that the
compiler will not play these games over inline asm, i.e. the index
will always be modified before use in all cases.