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 02/10/2019 09:24, Jan Beulich wrote:
> On 01.10.2019 17:37, Andrew Cooper 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
>>>> ...
>>>>
>>>> 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.
> I agree; since evaluate_nospec() insertion need is generally a hard
> to investigate / review action, I don#t consider this unexpected.
>
>>> 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.
> And all of which are because, other than I think it was originally
> intended, the functions still aren't always_inline.

Right, but if we force is_hvm_domain() to be always_inline, then
is_hvm_vcpu() gets out-of-lined.

This turns into a game of whack-a-mole, where any predicate wrapping
something with an embedded evaluate_nospec() breaks the safety.

~Andrew

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