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

Jan

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