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]

Message-ID: <CAPcyv4i6zOJZakNVW=y8NyUFB1EzOxBtoOGVhWCwysuTi6grvg@mail.gmail.com>
Date:   Tue, 22 May 2018 13:50:18 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     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>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] kernel: sys: fix potential Spectre v1

On Sun, May 20, 2018 at 7:00 PM, Gustavo A. R. Silva
<gustavo@...eddedor.com> wrote:
>
>
> On 05/20/2018 07:50 PM, Gustavo A. R. Silva wrote:
>>
>>
>>
>> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>>
>>>>>
>>>>> #ifndef sanitize_index_nospec
>>>>> inline bool sanitize_index_nospec(unsigned long *index,
>>>>>                                    unsigned long size)
>>>>> {
>>>>>          if (*index >= size)
>>>>>                  return false;
>>>>>          *index = array_index_nospec(*index, size);
>>>>>
>>>>>          return true;
>>>>> }
>>>>> #endif
>>>>
>>>>
>>>> I think this is fine in concept, we already do something similar in
>>>> mpls_label_ok(). Perhaps call it validate_index_nospec() since
>>>> validation is something that can fail, but sanitization in theory is
>>>> something that can always succeed.
>>>>
>>>
>>> OK. I got it.
>>>
>>>> However, the problem is the data type of the index. I expect you would
>>>> need to do this in a macro and use typeof() if you wanted this to be
>>>> generally useful, and also watch out for multiple usage of a macro
>>>> argument. Is it still worth it at that point?
>>>>
>>>
>>> Yeah. I think it is worth it. I'll work on this during the weekend and
>>> send a proper patch for review.
>>>
>>
>> Dan,
>>
>> What do you think about this first draft:
>>
>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>> index e791ebc..6154183 100644
>> --- a/include/linux/nospec.h
>> +++ b/include/linux/nospec.h
>> @@ -55,4 +55,16 @@ static inline unsigned long
>> array_index_mask_nospec(unsigned long index,
>>                                                                         \
>>          (typeof(_i)) (_i & _mask);                                     \
>>   })
>> +
>> +#define validate_index_nospec(index, size)                            \
>> +({                                                                    \
>> +       typeof(index) *ptr = &(index);                                 \
>> +       typeof(size) _s = (size);                                      \
>> +                                                                      \
>> +       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
>> +       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
>> +                                                                      \
>> +       *ptr >= _s ? false :                                           \
>> +       (*ptr = array_index_nospec(*ptr, _s) ? true : true);           \
>
>
> This actually should be:
>
> ((*ptr = array_index_nospec(*ptr, _s)) ? true : true);
>

Let's not use ternary conditionals at all to make this more readable.