Re: [PATCH SpectreV1+L1TF v4 05/11] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@amazon.de>]

On 1/23/19 14:37, Jan Beulich wrote:
>>>> On 23.01.19 at 12:51, <nmanthey@amazon.de> wrote:
>> @@ -1268,7 +1272,8 @@ unmap_common(
>>      }
>>  
>>      smp_rmb();
>> -    map = &maptrack_entry(lgt, op->handle);
>> +    map = &maptrack_entry(lgt, array_index_nospec(op->handle,
>> +                                                  lgt->maptrack_limit));
> It might be better to move this into maptrack_entry() itself, or
> make a maptrack_entry_nospec() clone (as several but not all
> uses may indeed not be in need of the extra protection). At
> least the ones in steal_maptrack_handle() and
> put_maptrack_handle() also look potentially suspicious.
I will move the nospec protection into the macro. I would like to avoid
introducing yet another macro.
>
>> @@ -2223,7 +2231,8 @@ gnttab_transfer(
>>          okay = gnttab_prepare_for_transfer(e, d, gop.ref);
>>          spin_lock(&e->page_alloc_lock);
>>  
>> -        if ( unlikely(!okay) || unlikely(e->is_dying) )
>> +        /* Make sure this check is not bypassed speculatively */
>> +        if ( evaluate_nospec(unlikely(!okay) || unlikely(e->is_dying)) )
>>          {
>>              bool_t drop_dom_ref = !domain_adjust_tot_pages(e, -1);
> What is it that makes this particular if() different from other
> surrounding ones? In particular the version dependent code (a few
> lines down from here as well as elsewhere) look to be easily
> divertable onto the wrong branch, then causing out of bounds
> speculative accesses due to the different (version dependent)
> shared entry sizes.
This check evaluates the variable okay, which indicates whether the
value of gop.ref is bounded correctly. The next conditional that uses
code based on a version should be fine, even when being entered
speculatively with the wrong version setup, as the value of gop.ref is
correct (i.e. architecturally visible after this lfence) already. As the
version dependent macros are used, i.e. shared_entry_v1 and
shared_entry_v2, I do not see a risk why speculative out-of-bound access
should happen here.
>
>> @@ -3215,6 +3230,10 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
>>      if ( ref_a == ref_b )
>>          goto out;
>>  
>> +    /* Make sure the above check is not bypassed speculatively */
>> +    ref_a = array_index_nospec(ref_a, nr_grant_entries(d->grant_table));
>> +    ref_b = array_index_nospec(ref_b, nr_grant_entries(d->grant_table));
> I think this wants to move up ahead of the if() in context, and the
> comment be changed to plural.
I will move the code above the comparison.
>
>> --- a/xen/include/xen/nospec.h
>> +++ b/xen/include/xen/nospec.h
>> @@ -87,6 +87,15 @@ static inline bool lfence_true(void) { return true; }
>>  #define evaluate_nospec(condition) ({ bool res = (condition); rmb(); res; 
>> })
>>  #endif
>>  
>> +/*
>> + * allow to block speculative execution in generic code
>> + */
> Comment style again.
I will fix the comment.
>
>> +#ifdef CONFIG_X86
>> +#define block_speculation() rmb()
>> +#else
>> +#define block_speculation()
>> +#endif
> Why does this not simply resolve to what currently is named lfence_true()
> (perhaps with a cast to void)? And why does this not depend on the
> Kconfig setting?

I will update the definition of this macro to what is called
lfence_true() in this series, and cast it to void. I will furthermore
split the introduction of this macro and this commit.

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