On 2/1/19 09:23, Jan Beulich wrote:
>>>> On 31.01.19 at 21:02, <andrew.coop...@citrix.com> 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?
Best,
Norbert
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
