Re: [PATCH] x86/spectre_v1: Disable compiler optimizations over array_index_mask_nospec()

[Mark Rutland]

Hi Dan,

On Fri, May 25, 2018 at 10:21:08AM -0700, Dan Williams wrote:
> Mark notes that gcc optimization passes have the potential to elide
> necessary invocations of this instruction sequence, so include an
> optimization barrier.
> 
>     > 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 certainly believe that we want the volatile.

However, just to be clear, the barrier doesn't prevent the above example, since
we don't currently have a mechanism to prevent the compiler splitting basic
blocks and inserting additional branches between those blocks.

I've written up a rationale for the volatile below, if you want something for
the commit message. There's a minor comment on the patch after that.

----
The volatile will inhibit *some* cases where the compiler could lift the
array_index_nospec() call out of a branch, e.g. where there are multiple
invocations of array_index_nospec() with the same arguments:

	if (idx < foo) {
		idx1 = array_idx_nospec(idx, foo)
		do_something(idx1);
	}

	< some other code >

	if (idx < foo) {
		idx2 = array_idx_nospec(idx, foo);
		do_something_else(idx2);
	}

... since the compiler can determine that the two invocations yield the same
result, and reuse the first result (likely the same register as idx was in
originally) for the second branch, effectively re-writing the above as:


	if (idx < foo) {
		idx = array_idx_nospec(idx, foo);
		do_something(idx);
	}

	< some other code >

	if (idx < foo) {
		do_something_else(idx);
	}
	
... if we don't take the first branch, then speculatively take the second, we
lose the nospec protection.

There's more info on volatile asm in the GCC docs:

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
----

> Cc: <stable@xxxxxxxxxxxxxxx>
> Fixes: babdde2698d4 ("x86: Implement array_index_mask_nospec")
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>  arch/x86/include/asm/barrier.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
> index 042b5e892ed1..41f7435c84a7 100644
> --- a/arch/x86/include/asm/barrier.h
> +++ b/arch/x86/include/asm/barrier.h
> @@ -38,10 +38,11 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  {
>  	unsigned long mask;
>  
> -	asm ("cmp %1,%2; sbb %0,%0;"
> +	asm volatile ("cmp %1,%2; sbb %0,%0;"
>  			:"=r" (mask)
>  			:"g"(size),"r" (index)
>  			:"cc");
> +	barrier();
>  	return mask;
>  }

What does the barrier() prevent?

I don't think that inhibits the insertion of branches, and AFAIK the volatile
is sufficient to prevent elision of identical array_idx_nospec() calls.

I don't have an objection to it, regardless.

So long as the example is updated in the commit message, feel free to add:

Acked-by: Mark Rutland <mark.rutland@xxxxxxx>

Thanks,
Mark.