Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/10/2019 13:21, Jan Beulich wrote:
> On 30.09.2019 20:24, Andrew Cooper wrote:
>> The code generation for barrier_nospec_true() is not correct.  We are taking 
>> a
>> perf it from the added fences, but not gaining any speculative safety.
> You want to be more specific here, I think: ISTR you saying that the LFENCEs
> get inserted at the wrong place.

Correct.

>  IIRC we want them on either side of a
> conditional branch, i.e. immediately following a branch insn as well as right
> at the branch target.

Specifically, they must be at the head of both basic blocks following
the conditional jump.

> I've taken, as a simple example,
> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
> code (in a non-debug build). Hence either I'm mis-remembering what we want
> things to look like, or there's more to it than code generation simply being
> "not correct".

This example demonstrates the problem, and actually throws a further
spanner in the works of how make this safe, which hadn't occurred to me
before.

The instruction stream from a caller of p2m_mem_access_sanity_check()
will look something like this:

call p2m_mem_access_sanity_check
    ...
    lfence
    ...
    ret   
cmp $0, %eax
jne ...

Which is unsafe, because the only safe way to arrange this code would be:

call p2m_mem_access_sanity_check
    ...
    ret
cmp $0, %eax
jne 1f
lfence
...
1: lfence
...

There is absolutely no possible way for inline assembly to be used to
propagate this safety property across translation units.  This is going
to have to be an attribute of some form or another handled by the compiler.

>
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>>  
>>        If unsure, say Y.
>>  
>> +config SPECULATIVE_BRANCH_HARDEN
>> +    bool "Speculative Branch Hardening"
>> +    depends on BROKEN
>> +        ---help---
>> +      Contemporary processors may use speculative execution as a
>> +      performance optimisation, but this can potentially be abused by an
>> +      attacker to leak data via speculative sidechannels.
>> +
>> +      One source of misbehaviour is by executing the wrong basic block
>> +      following a conditional jump.
>> +
>> +      When enabled, specific conditions which have been deemed liable to
>> +      be speculatively abused will be hardened to avoid entering the wrong
>> +      basic block.
>> +
>> +      !!! WARNING - This is broken and doesn't generate safe code !!!
> Not being a native speaker, this read to me as "is broken in general",
> whereas the brokenness is that according to your analysis safe code
> does not result. Therefore how about "This is broken in that it doesn't
> generate safe code"?

I wouldn't necessarily agree with the "in general" implication, but
given the lack of clarity, a better option would be:

!!! WARNING - This option doesn't work as intended. It does not generate
speculatively safe code !!!

>
>> --- a/xen/include/asm-x86/nospec.h
>> +++ b/xen/include/asm-x86/nospec.h
>> @@ -9,8 +9,8 @@
>>  /* Allow to insert a read memory barrier into conditionals */
>>  static always_inline bool barrier_nospec_true(void)
>>  {
>> -#ifdef CONFIG_HVM
>> -    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
>> +#ifdef CONFIG_SPECULATIVE_BRANCH_HARDEN
>> +    alternative("", "lfence", X86_FEATURE_ALWAYS);
> Why alternative() then and not just asm()?

Ok.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel