On 2/13/19 12:50, Jan Beulich wrote: >>>> On 08.02.19 at 14:44, <nmant...@amazon.de> 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? I believe a speculative v2 access can touch data that has been written by valid v1 accesses before, zero initialized data, or touch the NULL page. Given the macros for the access I do not believe that a v2 access can touch a page that is located behind a page holding valid v1 data. > >> @@ -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. I will move the "/" to the upper line, and add the space around the "%". > >> @@ -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. Instead of the block_speculation() macro, I can also protect the op->ref usage before evaluate_nospec via the array_index_nospec function. > > 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. The adjustment you propose is to exchange the switch statement in nr_grant_entries with a if( evaluate_nospec( gt->gt_version == 1 ), so that the returned values are not speculated? Already before this modification the function is called and not inlined. Do you want me to cache the value in functions that call this method regularly to avoid the penalty of the introduced lfence for each call? > > 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()?
Protecting the function itself should allow to not modify the speculation guards in these functions. I would have to check each of them, whether the guest actually has control, and whether it makes sense to introduce a _nospec variant of the nr_grant_entries function to not penalize everywhere. Do you have an opinion on this? Best, Norbert > > Jan > > Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich Ust-ID: DE 289 237 879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
