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

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.

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.

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