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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 31.01.19 at 21:02, <andrew.cooper3@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.)

>  As with all of these issues, you can only confirm whether
> you are no longer vulnerable by inspecting the eventual compiled code.

Which is nothing one can sensibly do, because any change (to
code or the tool chain) would immediately invalidate all of the
previously accumulated results.

Jan


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