Messages in this thread | | | Date | Wed, 23 May 2018 16:07:37 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH] kernel: sys: fix potential Spectre v1 |
| |
On Wed, May 23, 2018 at 11:08:40AM +0200, Peter Zijlstra wrote: > > Sorry for being late to the party..
Likewise!
> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote: > > +#define validate_index_nospec(index, size) \ > > +({ \ > > + bool ret = true; \ > > + typeof(index) *ptr = &(index); \ > > + typeof(size) _s = (size); \ > > + \ > > + BUILD_BUG_ON(sizeof(*ptr) > sizeof(long)); \ > > + BUILD_BUG_ON(sizeof(_s) > sizeof(long)); \ > > + \ > > + if (*ptr >= size) \ > > + ret = false; \ > > + \ > > + *ptr = array_index_nospec(*ptr, _s); \ > > + \ > > + ret; \ > > +}) > > Would not something like: > > bool ret = false; > > .... > > if (*ptr < _s) { > *ptr = array_index_nospec(*ptr, _s); > ret = true; > } > > ret; > > be more obvious?
I think that either way, we have a potential problem if the compiler generates a branch dependent on the result of validate_index_nospec().
In that case, we could end up with codegen approximating:
bool safe = false;
if (idx < bound) { idx = array_index_nospec(idx, bound); safe = true; }
// this branch can be mispredicted if (safe) { foo = array[idx]; }
... and thus we lose the nospec protection.
I also suspect that compiler transformations mean that this might already be the case for patterns like:
if (idx < bound) { safe_idx = array_index_nospec(idx, bound)]; ... foo = array[safe_idx]; }
... if the compiler can transform that to something like:
if (idx < bound) { idx = array_index_nospec(idx, bound); }
// can be mispredicted if (idx < bound) { foo = array[idx]; }
... which I think a compiler might be capable of, depending on the rest of the function body (e.g. if there's a common portion shared with the else case).
I'll see if I can trigger that in a test case. :/
Thanks, Mark.
| |