On 1/31/19 17:19, Jan Beulich wrote: >>>> On 29.01.19 at 15:43, <nmant...@amazon.de> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
