Re: [Xen-devel] [PATCH L1TF MDS GT v2 1/2] common/grant_table: harden bound accesses

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 7/11/19 14:34, Jan Beulich wrote:
> On 10.07.2019 14:54, Norbert Manthey wrote:
>> Guests can issue grant table operations and provide guest controlled
>> data to them. This data is used as index for memory loads after bound
>> checks have been done. To avoid speculative out-of-bound accesses, we
>> use the array_index_nospec macro where applicable, or the macro
>> block_speculation. Note, the block_speculation macro is used on all
>> path in shared_entry_header and nr_grant_entries. This way, after a
>> call to such a function, all bound checks that happened before become
>> architectural visible, so that no additional protection is required
>> for corresponding array accesses. As the way we introduce an lfence
>> instruction might allow the compiler to reload certain values from
>> memory multiple times, we try to avoid speculatively continuing
>> execution with stale register data by moving relevant data into
>> function local variables.
>>
>> 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
> Upon re-reading I think this last item isn't fully applicable: I think
> you attach such an attribute to domain creation/destruction functions.
> Those aren't executed repeatedly for a single guest, but e.g. rapid
> rebooting of a guest (or several ones) may actually be able to train
> such paths.
True, but a rebooted domain might come up on a different core, which
results in using different hardware structures. The "repeatedly" is open
to be interpreted, I agree. From my understanding, it belongs to the
properties to have to reliably target a speculative out-of-bound access.
>
>> @@ -2091,6 +2100,7 @@ gnttab_transfer(
>>       struct page_info *page;
>>       int i;
>>       struct gnttab_transfer gop;
>> +    grant_ref_t ref;
> This declaration would better live in the more narrow scope it's
> (only) used in.
I will move it accordingly, however, as discussed below, I'll drop this
one instead.
>
>> @@ -2237,8 +2247,14 @@ gnttab_transfer(
>>            */
>>           spin_unlock(&e->page_alloc_lock);
>>           okay = gnttab_prepare_for_transfer(e, d, gop.ref);
>> +        ref = gop.ref;
> Other than in the earlier cases here you copy a variable that's
> already local to the function. Is this really helpful?
I wanted to make the two as close as possible, I will change it back as
I did not find a difference in the binary.
>
> Independent of this - is there a particular reason you latch the
> value into the (second) local variable only after its first use? It
> likely won't matter much, but it's a little puzzling nevertheless.
I wanted to make the use of the new variable as close as possible to the
bound check and instruction blocking speculation.
>
>> -        if ( unlikely(!okay || assign_pages(e, page, 0, MEMF_no_refcount)) )
>> +        /*
>> +         * Make sure the reference bound check in 
>> gnttab_prepare_for_transfer
>> +         * is respected and speculative execution is blocked accordingly
>> +         */
>> +        if ( unlikely(!evaluate_nospec(okay)) ||
>> +            unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) )
> If I can trust my mail UI (which I'm not sure I can) there's an
> indentation issue here.
It looks fine to me, but I'll double check.
>
>> @@ -3853,7 +3879,8 @@ static int gnttab_get_status_frame_mfn(struct domain 
>> *d,
>>               return -EINVAL;
>>       }
>>   
>> -    *mfn = _mfn(virt_to_mfn(gt->status[idx]));
>> +    /* Make sure idx is bounded wrt nr_status_frames */
>> +    *mfn = _mfn(virt_to_mfn(gt->status[array_index_nospec(idx, 
>> nr_status_frames(gt))]));
> This and ...
>
>> @@ -3882,7 +3909,8 @@ static int gnttab_get_shared_frame_mfn(struct domain 
>> *d,
>>               return -EINVAL;
>>       }
>>   
>> -    *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
>> +    /* Make sure idx is bounded wrt nr_status_frames */
>> +    *mfn = _mfn(virt_to_mfn(gt->shared_raw[array_index_nospec(idx, 
>> nr_grant_frames(gt))]));
> ... this line are too long now and hence need wrapping.

I'll wrap the lines.

Best,
Norbert





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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