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

[Mark Rutland <mark.rutland@arm.com>]

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.