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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

I looked into these changes after a while again. I will split this
larger commit into smaller ones, and address parts of the problem in
each of them separately.

On 3/29/19 18:11, Jan Beulich wrote:
>>>> 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.
To be on the safe side, I will add a fix here 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.
That likely meant unmap_common_complete, and that one will be fixed.
>
>>  * 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.
I will address both.
>
>>  * 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.
That seems to be a left over. This function is actually fixed.
>
>>  * 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?
Yes, I think this is good enough.
>
>>  * 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?
I think this is save, yes.
>
>> @@ -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.
Yes, I will add an 'also' to these calls.
>
>> @@ -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?
That one should be fine, as it exists that function afterwards, and
represents an abort path that is valid for both versions.
>
>> @@ -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?

This idx access is to a version dependent structure, and hence
array_index_nospec is not good enough to catch the version difference
case as well.

Best,
Norbert





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