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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.02.19 at 09:16, <nmanthey@xxxxxxxxx> wrote:
> @@ -226,10 +228,18 @@ nr_maptrack_frames(struct grant_table *t)
>  static grant_entry_header_t *
>  shared_entry_header(struct grant_table *t, grant_ref_t ref)
>  {
> -    if ( t->gt_version == 1 )
> +    switch ( t->gt_version )
> +    {
> +    case 1:
> +        /* Make sure we return a value independently of speculative 
> execution */
> +        block_speculation();
>          return (grant_entry_header_t*)&shared_entry_v1(t, ref);
> -    else
> +    case 2:
> +        /* Make sure we return a value independently of speculative 
> execution */
> +        block_speculation();
>          return &shared_entry_v2(t, ref).hdr;
> +    }
> +    return NULL;
>  }

I'm not happy with the comment wording, as to me it reads ambiguously
at best. How about "Make sure the value returned is independent of
speculative execution"? I'm of course open to even better suggestions,
in particular by native speakers.

Also please add a blank line
- between the individual case blocks,
- before the final (main) return statement.

Plus please add ASSERT_UNREACHABLE() ahead of the NULL return.

> @@ -963,9 +979,15 @@ 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 bound check cannot be bypassed speculatively */
> +    block_speculation();
> +
>      act = active_entry_acquire(rgt, op->ref);
>      shah = shared_entry_header(rgt, op->ref);

So shared_entry_header() now has a fence before consuming op->ref.
Is there anything wrong with swapping these two and dropping the
extra fence you add above, like you do in acquire_grant_for_copy()? All
this would seem to require is adding block_speculation() also onto the
"return NULL" path of shared_entry_header() (where it shouldn't hurt
at all).

> @@ -2946,6 +2981,7 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>      grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>      int res;
>      unsigned int i;
> +    unsigned int gt_nr_grant_entries;

Rather then lengthening the name by adding a disambiguating prefix,
how about shortening it to "nr" or "nr_ents" (also elsewhere)? Also
please add onto the line declaring i instead of adding yet another line
with the same type.

> @@ -2969,7 +3005,8 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>       * are allowed to be in use (xenstore/xenconsole keeps them mapped).
>       * (You need to change the version number for e.g. kexec.)
>       */
> -    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
> +    gt_nr_grant_entries = nr_grant_entries(gt);
> +    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < gt_nr_grant_entries; i++ )

This then also calls for a 3rd block_speculation() in nr_grant_entries(),
I think.

Jan



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