Re: [Xen-devel] [PATCH SpectreV1+L1TF v5 3/9] x86/hvm: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.02.19 at 15:06, <nmanthey@xxxxxxxxx> wrote:
> On 2/1/19 09:23, Jan Beulich wrote:
>>>>> On 31.01.19 at 21:02, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 31/01/2019 16:19, Jan Beulich wrote:
>>>>> @@ -4104,6 +4108,12 @@ static int hvmop_set_param(
>>>>>      if ( a.index >= HVM_NR_PARAMS )
>>>>>          return -EINVAL;
>>>>>  
>>>>> +    /*
>>>>> +     * Make sure the guest controlled value a.index is bounded even 
>>>>> during
>>>>> +     * speculative execution.
>>>>> +     */
>>>>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>>>> I'd like to come back to this model of updating local variables:
>>>> Is this really safe to do? If such a variable lives in memory
>>>> (which here it quite likely does), does speculation always
>>>> recognize the update to the value? Wouldn't it rather read
>>>> what's currently in that slot, and re-do the calculation in case
>>>> a subsequent write happens? (I know I did suggest doing so
>>>> earlier on, so I apologize if this results in you having to go
>>>> back to some earlier used model.)
>>> I'm afraid that is a very complicated set of questions to answer.
>>>
>>> The processor needs to track write=>read dependencies to avoid wasting a
>>> large quantity of time doing erroneous speculation, therefore it does. 
>>> Pending writes which have happened under speculation are forwarded to
>>> dependant instructions.
>>>
>>> This behaviour is what gives rise to Bounds Check Bypass Store - a half
>>> spectre-v1 gadget but with a store rather than a write.  You can e.g.
>>> speculatively modify the return address on the stack, and hijack
>>> speculation to an attacker controlled address for a brief period of
>>> time.  If the speculation window is long enough, the processor first
>>> follows the RSB/RAS (correctly), then later notices that the real value
>>> on the stack was different, discards the speculation from the RSB/RAS
>>> and uses the attacker controlled value instead, then eventually notices
>>> that all of this was bogus and rewinds back to the original branch.
>>>
>>> Another corner case is Speculative Store Bypass, where memory
>>> disambiguation speculation can miss the fact that there is a real
>>> write=>read dependency, and cause speculation using the older stale
>>> value for a period of time.
>>>
>>>
>>> As to overall safety, array_index_nospec() only works as intended when
>>> the index remains in a register between the cmp/sbb which bounds it
>>> under speculation, and the array access.  There is no way to guarantee
>>> this property, as the compiler can spill any value if it thinks it needs to.
>>>
>>> The general safety of the construct relies on the fact that an
>>> optimising compiler will do its very best to avoid spilling variable to
>>> the stack.
>> "Its very best" may be extremely limited with enough variables.
>> Even if we were to annotate them with the "register" keyword,
>> that still wouldn't help, as that's only a hint. We simply have no
>> way to control which variables the compiler wants to hold in
>> registers. I dare to guess that in the particular example above
>> it's rather unlikely to be put in a register.
>>
>> In any event it looks like you support my suspicion that earlier
>> comments of mine may have driven things into a less safe
>> direction, and we instead need to accept the more heavy
>> clutter of scattering around array_{access,index}_nospec()
>> at all use sites instead of latching the result of
>> array_index_nospec() into whatever shape of local variable.
>>
>> Which raises another interesting question: Can't CSE and
>> alike get in the way here? OPTIMIZER_HIDE_VAR() expands
>> to a non-volatile asm() (and as per remarks elsewhere I'm
>> unconvinced adding volatile would actually help), so the
>> compiler recognizing the same multiple times (perhaps in a
>> loop) could make it decide to calculate the thing just once.
>> array_index_mask_nospec() in effect is a pure (and actually
>> even const) function, and the lack of a respective attribute
>> doesn't make the compiler not treat it as such if it recognized
>> the fact. (In effect what I had asked Norbert to do to limit
>> the clutter was just CSE which the compiler may or may not
>> have recognized anyway. IOW I'm not convinced going back
>> would actually buy us anything.)
> 
> So this means I should stick to the current approach and continue
> updating variables after their bound check with an array_index_nospec
> call, correct?

Well, yes, at least for now I'm not convinced going back and
re-introduce the heavier code churn would buy us much. But
we'll have to see whether e.g. Andrew is of a different opinion.

Jan


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