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

[Dan Williams]

On Sun, May 20, 2018 at 7:00 PM, Gustavo A. R. Silva
<gustavo@xxxxxxxxxxxxxx> 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.