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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 5/23/19 16:17, Jan Beulich wrote:
>>>> On 21.05.19 at 09:45, <nmanthey@xxxxxxxxx> 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, that the block_speculation is always used in
> s/always/already/ ?
They both work, but the 'always' underlines that a caller can rely on
the fact that this will happen on all execution path of that function.
Hence, I like to stick to 'always' here.
>
>> the calls to shared_entry_header and nr_grant_entries, so that no
>> additional protection is required once these functions have been
>> called.
> Isn't this too broad a statement? There's some protection, but not
> for just anything that follows.
You are right, to given protection is that any bound check that could
have been bypassed speculatively is enforced after calling one of the
two functions. I will rephrase the commit message accordingly.
>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -988,9 +988,10 @@ 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 also ensures the above check cannot be passed 
>> speculatively */
>>      shah = shared_entry_header(rgt, op->ref);
>>      status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, 
>> op->ref);
>> +    act = active_entry_acquire(rgt, op->ref);
> I know we've been there before, but what guarantees that the
> compiler won't reload op->ref from memory for either of the
> latter two accesses? In fact afaict it always will, due to the
> memory clobber in alternative().
The compiler can reload op->ref from memory, that is fine here, as the
bound check happens above, and the shared_entry call comes with an
lfence() by now, so that we will not continue executing speculatively
with op->ref being out-of-bounds, independently of whether it's from
memory or registers.
>
>> @@ -3863,6 +3883,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]));
>>      return 0;
>>  }
> Why don't you use array_index_nospec() here? And how come
There is no specific reason. I will swap.
> speculation into gnttab_grow_table() is fine a few lines above?
I do not see a reason why it would be bad to enter that function
speculatively. There are no accesses that would have to be protected by
extra checks, afaict. Otherwise, that function should be protected by
its own.
> And what about the similar code in gnttab_get_shared_frame_mfn()?
I will add an array_nospec_index there as well.
>
> 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