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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.05.19 at 11:54, <nmanthey@xxxxxxxxx> wrote:
> 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.

Well, I'm not a native speaker, but to me "always" doesn't express
what you want to express here. What you mean I'd word "... is used
on all paths of ..."

>>> the calls to shared_entry_header and nr_grant_entries, so that no
>>> additional protection is required once these functions have been
>>> called.

As an aside, your use of "in the calls to" looks also misleading to me,
because the fences sit in the functions, not at the call sites.

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

I don't buy this argumentation: In particular if the cache line got
flushed for whatever reason, the load may take almost arbitrarily
long, opening up a large speculation window again using the
destination register of the load (whatever - not bounds checked -
value that ends up holding).

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

Which in fact happens, but only in patch 3. This may be worth saying
explicitly here.

Jan



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