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

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 05/18/2018 04:45 PM, Dan Williams wrote:
> On Fri, May 18, 2018 at 2:27 PM, Gustavo A. R. Silva
> <gustavo@embeddedor.com> wrote:
>>
>>
>> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>>>
>>>>>>
>>>>>
>>>>> Oops, it seems I sent the wrong patch. The function would look like
>>>>> this:
>>>>>
>>>>> #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.
>>>
>>> Thanks for the feedback.
>>
>>
>> BTW, I'm analyzing other cases, like the following:
>>
>> bool foo(int x)
>> {
>>           if(!validate_index_nospec(&x))
>>                   return false;
>>
>>           [...]
>>
>>           return true;
>> }
>>
>> int vulnerable(int x)
>> {
>>           if (!foo(x))
>>                   return -1;
>>
>>           temp = array[x];
>>
>>           [...]
>>
>> };
>>
>> Basically my doubt is how deep this barrier can be placed into the call
>> chain in order to continue working.
> 
> This is broken you would need to pass the address of x into foo()
> otherwise there may be speculation on the return value of foo.
> 

Oh I see now. Just to double check, then something like the following 
would be broken too, because is basically the same as the code above, 
and well, it doesn't make much sense to store the value returned by 
macro array_index_nospec into x, correct?:

bool foo(int x)
{
	if(x >= MAX)
		return false;

	x = array_index_nospec(x, MAX);
	return true;
}

int vulnerable(int x)
{
	if(!foo(x))
		return -1;

	temp = array[x];

	[...]
}

Thanks
--
Gustavo