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

[Gustavo A. R. Silva]

On 05/23/2018 11:31 AM, Mark Rutland wrote:
> On Wed, May 23, 2018 at 04:07:37PM +0100, Mark Rutland wrote:
> > 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 see GCC do this at -O0, but so far I haven't tricked it into doing
> this at -O1 or above.
>
> Regardless, I worry this is fragile -- GCC *can* generate code as per
> the above, even if it's unlikely to.
>
> > 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. :/
>
> No luck so far, but I'll keeep fighting...
>
> GCC will happily pull a common suffix after the branch, e.g.
>
> 	if (cond) {
> 		foo();
> 		bar();
> 	} else {
> 		bar();
> 	}
>
> .. goes to:
>
> 	if (cond)
> 		foo()
>
> 	bar();
>
> ... but I can't convince it to pull a common prefix before the branch.
>
> Mark.
I will send the following patch once Dan's [1] has been applied upstream.

diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index e791ebc..2a1ab2e 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -55,4 +55,21 @@ static inline unsigned long 
array_index_mask_nospec(unsigned long index,
                                                                       \
        (typeof(_i)) (_i & _mask);                                     \
 })
+
+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       bool ret = false;                                              \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       if (*ptr < _s) {                                               \
+               *ptr = array_index_nospec(*ptr, _s);                   \
+               ret = true;                                            \
+       }                                                              \
+                                                                      \
+       ret;                                                           \
+})

[1] https://marc.info/?l=linux-kernel&m=152726947109104&w=2

Thank you, Dan, Peter and Mark for your feedback.
--
Gustavo