Re: [Xen-devel] [PATCH SpectreV1+L1TF v5 8/9] common/grant_table: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> wrote:
> @@ -963,6 +965,9 @@ map_grant_ref(
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>                   op->ref, rgt->domain->domain_id);
>  
> +    /* Make sure the above check is not bypassed speculatively */
> +    op->ref = array_index_nospec(op->ref, nr_grant_entries(rgt));
> +
>      act = active_entry_acquire(rgt, op->ref);
>      shah = shared_entry_header(rgt, op->ref);
>      status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, 
> op->ref);

Just FTR - this is a case where the change, according to prior
discussion, is pretty unlikely to help at all. The compiler will have
a hard time realizing that it could keep the result in a register past
the active_entry_acquire() invocation, as that - due to the spin
lock acquired there - acts as a compiler barrier. And looking at
generated code (gcc 8.2) confirms that there's a reload from the
stack.

> @@ -2026,6 +2031,9 @@ gnttab_prepare_for_transfer(
>          goto fail;
>      }
>  
> +    /* Make sure the above check is not bypassed speculatively */
> +    ref = array_index_nospec(ref, nr_grant_entries(rgt));
> +
>      sha = shared_entry_header(rgt, ref);
>  
>      scombo.word = *(u32 *)&sha->flags;
> @@ -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)) )

I'm still not really happy about this. The comment isn't helpful in
connecting the use of evaluate_nospec() to the problem site
(in the earlier hunk, which I've left in context), and I still don't
understand why the e->is_dying is getting wrapped as well.
Plus it occurs to me now that you're liable to render unlikely()
ineffective here. So how about

        if ( unlikely(evaluate_nospec(!okay)) || unlikely(e->is_dying) )

?

Jan



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