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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 2/13/19 12:50, Jan Beulich wrote:
>>>> 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?
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel