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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 May 2018 08:57:59 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH] kernel: sys: fix potential Spectre v1

On Wed, May 23, 2018 at 8:07 AM, Mark Rutland <mark.rutland@....com> 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.