Re: [Xen-devel] [PATCH L1TF v10 7/8] common/grant_table: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.19 at 13:50, <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.
> 
> Speculative execution is not blocked in case one of the following
> properties is true:
>  - path cannot be triggered by the guest
>  - path does not return to the guest
>  - path does not result in an out-of-bound access
>  - path cannot be executed repeatedly
> Only the combination of the above properties allows to actually leak
> continuous chunks of memory. Therefore, we only add the penalty of
> protective mechanisms in case a potential speculative out-of-bound
> access matches all the above properties.
> 
> 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 perform out-of-bound accesses of version 2 while
> the table is actually using version 1. Hence, speculation is prevented
> when accessing new memory based on the grant table version. In cases,
> where no different memory locations are accessed on the code path that
> follow an if statement, no protection is required. No different memory
> locations are accessed in the following functionsi after a version check:
> 
>  * _set_status, as the header memory layout is the same

Isn't this rather by virtue of shared_entry_header() having got
hardened? I don't think the memory layout alone can serve as a
reason for there to be no issue - the position in memory matters
as well.

>  * unmap_common, as potentially touched memory locations are allocated
>                  and initialized

I can't seem to spot any explicit version checks in unmap_common().
Do you mean unmap_common_complete()? If so I'm afraid I don't
understand what "allocated and initialized" is supposed to mean.
The version check there looks potentially problematic to me, at
least from a purely theoretical pov.

>  * gnttab_grow_table, as the touched memory is the same for each
>                 branch after the conditionals

How that? gnttab_populate_status_frames() could be speculated
into for a v1 guest.

Next there's a version check in gnttab_setup_table(), but the function
doesn't get changed and also isn't listed here.

>  * gnttab_transfer, as no memory access depends on the conditional
>  * release_grant_for_copy, as no out-of-bound access depends on this
>                 conditional

But you add evaluate_nospec() there, and memory accesses very well
look to depend on the condition, just not inside the bodies of the if/else.

>  * gnttab_set_version, as in case of a version change all the memory is
>                 touched in both cases

And you're sure speculation through NULL pointers is impossible? And
the offset-into-table differences between v1 and v2 don't matter?

>  * gnttab_release_mappings, as this function is called only during domain
>                 destruction and control is not returned to the guest
>  * mem_sharing_gref_to_gfn, as potential dangerous memory accesses are
>                 covered by the next evaluate_nospec
>  * gnttab_get_status_frame, as the potential dangerous memory accesses
>                 are protected in gnttab_get_status_frame_mfn

But there's quite a bit of code in gnttab_get_status_frame_mfn()
before the addition you make. But I guess speculation in particular
into gnttab_grow_table() might be safe?

> @@ -963,9 +988,13 @@ map_grant_ref(
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>                   op->ref, rgt->domain->domain_id);
>  
> -    act = active_entry_acquire(rgt, op->ref);
> +    /* This call ensures the above check cannot be bypassed speculatively */
>      shah = shared_entry_header(rgt, op->ref);

I know I've come across this several times by now, but I'm afraid I
now get the impression that the comment kind of suggests that
the call is just for this purpose, instead of fulfilling the purpose as
a side effect. Would you mind adding "also" to this (and possible
further instances)? To avoid the line growing too long, perhaps
"call" could be dropped instead.

> @@ -2410,9 +2448,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;

What about the second version check further down in this function?

> @@ -3838,6 +3886,9 @@ static int gnttab_get_status_frame_mfn(struct domain *d,
>              return -EINVAL;
>      }
>  
> +    /* Make sure idx is bounded wrt nr_status_frames */
> +    block_speculation();
> +
>      *mfn = _mfn(virt_to_mfn(gt->status[idx]));

Why not array_access_nospec()? And how is this different from
gnttab_get_shared_frame_mfn(), which you don't change?

Jan


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