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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2019 14:51, Andrew Cooper wrote:
> 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.

But you realize that this particular example is basically a more
complex is_XYZ() check, which could be dealt with by inlining the
function. Of course there are going to be larger functions where
the result wants to be guarded like you say. But just like the
addition of the nospec macros to various is_XYZ() functions is a
manual operation (as long the compiler doesn't help), it would in
that case be a matter of latching the return value into a local
variable and using an appropriate guarding construct when
evaluating it.

So I'm afraid for now I still can't agree with your "not correct"
assessment - the generated code in the example looks correct to
me, and if further guarding was needed in users of this particular
function, it would be those users which would need further
massaging.

>>> --- 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 !!!

Fine with me.

Jan

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