Re: [Xen-devel] [PATCH L1TF MDS GT v1 2/3] common/grant_table: harden bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.05.19 at 09:45, <nmanthey@xxxxxxxxx> wrote:
> Guests can issue grant table operations and provide guest controlled
> data to them. This data is used as index for memory loads after bound
> checks have been done. To avoid speculative out-of-bound accesses, we
> use the array_index_nospec macro where applicable, or the macro
> block_speculation. Note, that the block_speculation is always used in

s/always/already/ ?

> the calls to shared_entry_header and nr_grant_entries, so that no
> additional protection is required once these functions have been
> called.

Isn't this too broad a statement? There's some protection, but not
for just anything that follows.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -988,9 +988,10 @@ map_grant_ref(
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>                   op->ref, rgt->domain->domain_id);
>  
> -    act = active_entry_acquire(rgt, op->ref);
> +    /* This call also ensures the above check cannot be passed speculatively 
> */
>      shah = shared_entry_header(rgt, op->ref);
>      status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, 
> op->ref);
> +    act = active_entry_acquire(rgt, op->ref);

I know we've been there before, but what guarantees that the
compiler won't reload op->ref from memory for either of the
latter two accesses? In fact afaict it always will, due to the
memory clobber in alternative().

> @@ -3863,6 +3883,9 @@ static int gnttab_get_status_frame_mfn(struct domain *d,
>              return -EINVAL;
>      }
>  
> +    /* Make sure idx is bounded wrt nr_status_frames */
> +    block_speculation();
> +
>      *mfn = _mfn(virt_to_mfn(gt->status[idx]));
>      return 0;
>  }

Why don't you use array_index_nospec() here? And how come
speculation into gnttab_grow_table() is fine a few lines above?
And what about the similar code in gnttab_get_shared_frame_mfn()?

Jan



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