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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
