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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.10.2019 21:31, Andrew Cooper wrote:
> 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.

That's understood, but what do you do? The consequence is that we'd
have to go through _all_ inline (predicate) functions, converting
them to always_inline as needed. Auditing non-inline ones (like
p2m_mem_access_sanity_check()) would need doing independently anyway,
and I'd consider this an independent aspect/issue.

Jan

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