Messages in this thread | | | From | Dan Williams <> | Date | Tue, 22 May 2018 13:50:18 -0700 | Subject | Re: [PATCH] kernel: sys: fix potential Spectre v1 |
| |
On Sun, May 20, 2018 at 7:00 PM, Gustavo A. R. Silva <gustavo@embeddedor.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.
| |