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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 1/31/19 17:19, Jan Beulich wrote:
>>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> wrote:
>> There are multiple arrays in the HVM interface that are accessed
>> with indices that are provided by the guest. To avoid speculative
>> out-of-bound accesses, we use the array_index_nospec macro.
>>
>> When blocking speculative out-of-bound accesses, we can classify arrays
>> into dynamic arrays and static arrays. Where the former are allocated
>> during run time, the size of the latter is known during compile time.
>> On static arrays, compiler might be able to block speculative accesses
>> in the future.
>>
>> We introduce another macro that uses the ARRAY_SIZE macro to block
>> speculative accesses. For arrays that are statically accessed, this macro
>> can be used instead of the usual macro. Using this macro results in more
>> readable code, and allows to modify the way this case is handled in a
>> single place.
> I think this paragraph is stale now.
I will drop the paragraph.
>
>> @@ -3453,7 +3456,8 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t 
>> *msr_content)
>>          if ( (index / 2) >=
>>               MASK_EXTR(v->arch.hvm.mtrr.mtrr_cap, MTRRcap_VCNT) )
>>              goto gp_fault;
>> -        *msr_content = var_range_base[index];
>> +        *msr_content = var_range_base[array_index_nospec(index,
>> +                          MASK_EXTR(v->arch.hvm.mtrr.mtrr_cap, 
>> MTRRcap_VCNT))];
>>          break;
> I clearly should have noticed this earlier on - the bound passed into
> the macro is not in line with the if() condition. I think you're funneling
> half the number of entries into array slot 0.
I will fix the bound that's used in the array_index_nospec macro.
>
>> @@ -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 will reply to this on the thread that evolved.

Best,
Norbert

>
> Jan
>
>




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