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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 2/25/19 17:46, Jan Beulich wrote:
>>>> On 25.02.19 at 14:34, <nmanthey@xxxxxxxxx> wrote:
>> @@ -634,14 +649,24 @@ static unsigned int nr_grant_entries(struct 
>> grant_table *gt)
>>      case 1:
>>          BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 1) <
>>                       GNTTAB_NR_RESERVED_ENTRIES);
>> +
>> +        /* Make sure we return a value independently of speculative 
>> execution */
>> +        block_speculation();
>>          return f2e(nr_grant_frames(gt), 1);
>> +
>>      case 2:
>>          BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 2) <
>>                       GNTTAB_NR_RESERVED_ENTRIES);
>> +
>> +        /* Make sure we return a value independently of speculative 
>> execution */
>> +        block_speculation();
>>          return f2e(nr_grant_frames(gt), 2);
>>  #undef f2e
>>      }
>>  
>> +    block_speculation();
>> +    ASSERT_UNREACHABLE();
>> +
>>      return 0;
>>  }
> Personally I think the assertion should be first (also in
> shared_entry_header()), but that's nothing very important to
> change.
I will change the order.
>
> Below here, but before the next patch hunk, is _set_status(). If
> you think there's no need to change its gt_version check, then I
> think the commit message should (briefly) explain this.
>
>> @@ -1418,7 +1450,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
>>      struct page_info *pg;
>>      uint16_t *status;
>>  
>> -    if ( !op->done )
>> +    if ( evaluate_nospec(!op->done) )
>>      {
>>          /* unmap_common() didn't do anything - nothing to complete. */
>>          return;
> Just like above, below here (in the same function) is another version
> check you don't adjust, and there are further ones in gnttab_grow_table(),
> gnttab_setup_table(), and release_grant_for_copy().
>
>> @@ -2408,9 +2445,11 @@ acquire_grant_for_copy(
>>          PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
>>                   "Bad grant reference %#x\n", gref);
>>  
>> -    act = active_entry_acquire(rgt, gref);
>> +    /* This call ensures the above check cannot be bypassed speculatively */
>>      shah = shared_entry_header(rgt, gref);
>> -    if ( rgt->gt_version == 1 )
>> +    act = active_entry_acquire(rgt, gref);
>> +
>> +    if ( evaluate_nospec(rgt->gt_version == 1) )
>>      {
>>          sha2 = NULL;
>>          status = &shah->flags;
> There's again a second version check further down in this function.
>
>> @@ -2945,7 +2987,7 @@ 
>> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>>      struct grant_table *gt = currd->grant_table;
>>      grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>>      int res;
>> -    unsigned int i;
>> +    unsigned int i, nr_ents;
>>  
>>      if ( copy_from_guest(&op, uop, 1) )
>>          return -EFAULT;
>> @@ -2969,7 +3011,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++ )
>> +    nr_ents = nr_grant_entries(gt);
>> +    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_ents; i++ )
>>      {
>>          if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
>>          {
> What about the various version accesses in this function? And
> while I think the one in gnttab_release_mappings() doesn't need
> adjustment, it should (also) be mentioned in the description. The
> one in gnttab_map_frame(9, otoh, looks as if it again needed
> adjustment.
>
> I would really like to ask that I (or someone else) don't need to
> go through and list remaining version checks again - after all I
> had done so for v6 already, and I didn't go through all of them
> again for v7 assuming that you would have worked through the
> entire set.

So, here is the annotation for all of them. Anyone that I did not
include in the list has been fixed in previous versions, or will be
fixed in the next version:

git grep -np version common/grant_table.c

common/grant_table.c:831:static int _set_status(unsigned gt_version,
common/grant_table.c:840:    if ( gt_version == 1 )

-> I do not see how out-of-bound accesses happen in the called functions
there.

common/grant_table.c=1444=unmap_common_complete(struct
gnttab_unmap_common *op)
common/grant_table.c:1469:    if ( rgt->gt_version == 1 )

-> I do not see how to be exploitable, as the shared_entry_header call
above just used an lfence.

common/grant_table.c=1761=gnttab_grow_table(struct domain *d, unsigned
int req_nr_frames)
common/grant_table.c:1800:    /* Status pages - version 2 */
common/grant_table.c:1801:    if ( gt->gt_version > 1 )

-> I do not see how out-of-bound access could happen. This calls
gnttab_populate_status_frames that allocates pages and should not touch
more memory than before

common/grant_table.c=1904=gnttab_setup_table(
common/grant_table.c:1955:          ((gt->gt_version > 1) &&

-> I do not see how an out-of-bound access is possible.

common/grant_table.c=2104=gnttab_transfer(
common/grant_table.c:2199:            e, e->grant_table->gt_version > 1
|| paging_mode_translate(e)

-> I do not see dependent out-of-bound accesses.

common/grant_table.c=2337=release_grant_for_copy(
common/grant_table.c:2354:    if ( rgt->gt_version == 1 )
common/grant_table.c=2420=acquire_grant_for_copy(
common/grant_table.c:2452:    if ( evaluate_nospec(rgt->gt_version == 1) )
common/grant_table.c:2535:        if ( rgt->gt_version != 2 ||

-> I do not see dependent out-of-bound accesses.

common/grant_table.c=2982=static long
common/grant_table.c:2983:gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t)
uop)

-> in case of a version change, both fields are touched, and potential
out-of-bound accesses can be performed once, as the memory is allocated
afterwards and not freed during domain life time.

common/grant_table.c=3618=gnttab_release_mappings(
common/grant_table.c:3657:        if ( rgt->gt_version == 1 )

-> Is only called during domain destruction.

common/grant_table.c=3809=int mem_sharing_gref_to_gfn(struct grant_table
*gt, grant_ref_t ref,
common/grant_table.c:3817:    if ( gt->gt_version < 1 )

-> The next evaluate_nospec in the code covers the relevant memory accesses.

common/grant_table.c=3966=int gnttab_get_status_frame(struct domain *d,
unsigned long idx,
common/grant_table.c:3973:    rc = (gt->gt_version == 2) ?

-> The out-of-bound access will be blocked by using block_speculation in
gnttab_get_status_frame_mfn.

common/grant_table.c=3980=static void gnttab_usage_print(struct domain *rd)
common/grant_table.c:3994:           rd->domain_id, gt->gt_version,
common/grant_table.c:4015:        if ( gt->gt_version == 1 )

-> This function cannot be triggered by a guest.

>
> Mentioning reasons of omitted adjustments is in particular
> important for people to have a reference down the road, to be
> able to tell whether new version checks to add need to take one
> shape or the other.

I understand that his is important for future modifications. I will
extend the commit message with the reasons when not to block speculation
wrt the version check, i.e. cannot be triggered by the guest, does not
return to the guest, does not result in an out-of-bound access or cannot
be executed repeatedly.

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