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

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

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

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

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

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

> +#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?

Jan



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