Messages in this thread | | | From | Dan Williams <> | Date | Fri, 18 May 2018 15:08:09 -0700 | Subject | Re: [PATCH] kernel: sys: fix potential Spectre v1 |
| |
On Fri, May 18, 2018 at 3:01 PM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > > > 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?:
Correct, broken:
> > bool foo(int x) > { > if(x >= MAX) > return false;
Under speculation we may not return here when x is greater than max.
> x = array_index_nospec(x, MAX);
x is now sanitized under speculation to zero, but the compiler would likely just throw this away because nothing consumes it.
> return true; > } > > int vulnerable(int x) > { > if(!foo(x)) > return -1;
cpu might speculate that this branch is not taken...
> > temp = array[x];
...so x had better be bounded here, otherwise Spectre.
| |