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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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.

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.

Jan



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