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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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.

> @@ -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.)

Jan



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