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

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 1. Oct 2019, at 17:37, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> On 01/10/2019 15:32, Jan Beulich wrote:
>> 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
>>> …
>>> 

Does it mean the CPU speculates over a direct call (assuming no #PF etc) and
assumes some default return value to be used?

If not, maybe we should be more concerned about the value the cache-loading
gadget speculates with, rather than the sheer speculation over the branch.

Am I mis(understanding) something here? I would be thankful for explanation.

>>> 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.
> 
> And this reasoning demonstrates yet another problem (this one was raised
> at the meeting in Chicago).
> 
> evaluate_nospec() is not a useful construct if it needs inserting at
> every higher level predicate to result in safe code.  This is
> boarderline-impossible to review for, and extremely easy to break
> accidentally.
> 
>> 
>> 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.
> 
> Safety against spectre v1 is not a matter of opinion.
> 

But the hardening wasn’t about spectre v1, but about cache-load gadgets?

> To protect against speculatively executing the wrong basic block, the
> pipeline must execute the conditional jump first, *then* hit an lfence
> to serialise the instruction stream and revector in the case of
> incorrect speculation.

That’s true, but there are also 2 aspects worth mentioning:
1) Example:

jne 1
PRIVILEGED
1:
ALWAYS_SAFE

We do not necessarily have to cover the 1: path with an lfence?
We could allow speculation there, as it is harmless.

2) cache-load protection

It might be ok to speculate over a conditional branch, when we can
guarantee that the value to be used in a dereference is not out-of-bound.
In that case an lfence is used to latch the value in the register. We can
still speculate over the branch and reach the dereference, but with a sane 
value.

I agree that lfence might not give us 100% security in every potential case,
but that is what "hardening" gives you...

> 
> The other way around is not safe.  Serialising the instruction stream
> doesn't do anything to protect against the attacker taking control of a
> later branch.
> 

Sure, but it may do something about the value used to dereference memory
when the speculation happens over the branch.

> The bigger problem is to do with classifying what we are protecting
> against.  In the case of is_control_domain(), it is any action based on
> the result of the decision.  For is_{pv,hvm}_domain(), is only (to a
> first approximation) speculative type confusion into the pv/hvm unions
> (which in practice extends to calling pv_/hvm_ functions as well).
> 
> As for the real concrete breakages.  In a staging build with GCC 6
> 
> $ objdump -d xen-syms | grep '<is_hvm_domain>:' | wc -l
> 18
> $ objdump -d xen-syms | grep '<is_pv_domain>:' | wc -l
> 9
> 
> All of which have the lfence too early to protect against speculative
> type confusion.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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