[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>>> 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)                
>   \
> +                                     

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

> @@ -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()?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.