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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.19 at 14:44, <nmanthey@xxxxxxxxx> wrote:
> Guests can issue grant table operations and provide guest controlled
> data to them. This data is also used for memory loads. To avoid
> speculative out-of-bound accesses, we use the array_index_nospec macro
> where applicable. However, there are also memory accesses that cannot
> be protected by a single array protection, or multiple accesses in a
> row. To protect these, a nospec barrier is placed between the actual
> range check and the access via the block_speculation macro.
> 
> As different versions of grant tables use structures of different size,
> and the status is encoded in an array for version 2, speculative
> execution might touch zero-initialized structures of version 2 while
> the table is actually using version 1.

Why zero-initialized? Did I still not succeed demonstrating to you
that speculation along a v2 path can actually overrun v1 arrays,
not just access parts with may still be zero-initialized?

> @@ -203,8 +204,9 @@ static inline unsigned int nr_status_frames(const struct 
> grant_table *gt)
>  }
>  
>  #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
> -#define maptrack_entry(t, e) \
> -    ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
> +#define maptrack_entry(t, e)                                                 
>   \
> +    ((t)->maptrack[array_index_nospec(e, (t)->maptrack_limit)                
>   \
> +                                     
> /MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])

I would have hoped that the pointing out of similar formatting
issues elsewhere would have had an impact here as well, but
I see the / is still wrongly at the beginning of a line, and is still
not followed by a blank (would be "preceded" if it was well
placed). And while I realize it's only code movement, adding
the missing blanks around % would be appreciated too at this
occasion.

> @@ -963,9 +965,13 @@ 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 */
> +    block_speculation();
> +
>      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);
> +    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
> +                                                 : &status_entry(rgt, 
> op->ref);

Did you consider folding the two pairs of fences you emit into
one? Moving up the assignment to status ought to achieve this,
as then the block_speculation() could be dropped afaict.

Then again you don't alter shared_entry_header(). If there's
a reason for you not having done so, then a second fence
here is needed in any event.

What about the version check in nr_grant_entries()? It appears
to me as if at least its use in grant_map_exists() (which simply is
the first one I've found) is problematic without an adjustment.
Even worse, ...

> @@ -1321,7 +1327,8 @@ unmap_common(
>          goto unlock_out;
>      }
>  
> -    act = active_entry_acquire(rgt, op->ref);
> +    act = active_entry_acquire(rgt, array_index_nospec(op->ref,
> +                                                       
> nr_grant_entries(rgt)));

... you add a use e.g. here to _guard_ against speculation.

And what about _set_status(), unmap_common_complete(),
gnttab_grow_table(), gnttab_setup_table(),
release_grant_for_copy(), the 2nd one in acquire_grant_for_copy(),
several ones in gnttab_set_version(), gnttab_release_mappings(),
the 3rd one in mem_sharing_gref_to_gfn(), gnttab_map_frame(),
and gnttab_get_status_frame()?

Jan



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