Re: [PATCH SpectreV1+L1TF v5 8/9] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@amazon.de>]

On 2/7/19 15:00, Jan Beulich wrote:
>>>> On 07.02.19 at 11:20, <nmanthey@amazon.de> wrote:
>> On 2/7/19 10:50, Norbert Manthey wrote:
>>> On 2/6/19 16:53, Jan Beulich wrote:
>>>>>>> On 06.02.19 at 16:06, <nmanthey@amazon.de> wrote:
>>>>> On 2/6/19 15:52, Jan Beulich wrote:
>>>>>>>>> On 29.01.19 at 15:43, <nmanthey@amazon.de> wrote:
>>>>>>> @@ -963,6 +965,9 @@ map_grant_ref(
>>>>>>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>>>>>                   op->ref, rgt->domain->domain_id);
>>>>>>>  
>>>>>>> +    /* Make sure the above check is not bypassed speculatively */
>>>>>>> +    op->ref = array_index_nospec(op->ref, nr_grant_entries(rgt));
>>>>>>> +
>>>>>>>      act = active_entry_acquire(rgt, op->ref);
>>>>>>>      shah = shared_entry_header(rgt, op->ref);
>>>>>>>      status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, op->ref);
>>>>>> Just FTR - this is a case where the change, according to prior
>>>>>> discussion, is pretty unlikely to help at all. The compiler will have
>>>>>> a hard time realizing that it could keep the result in a register past
>>>>>> the active_entry_acquire() invocation, as that - due to the spin
>>>>>> lock acquired there - acts as a compiler barrier. And looking at
>>>>>> generated code (gcc 8.2) confirms that there's a reload from the
>>>>>> stack.
>>>>> I could change this back to a prior version that protects each read
>>>>> operation.
>>>> That or use block_speculation() with a comment explaining why.
>>>>
>>>> Also - why are there no changes at all to the unmap_grant_ref() /
>>>> unmap_and_replace() call paths? Note in particular the security
>>>> related comment next to the bounds check of op->ref there. I've
>>>> gone through earlier review rounds, but I couldn't find an indication
>>>> that this might have been the result of review feedback.
>>> You are right. I am not sure whether I had a fix placed there in the
>>> beginning. I will replace the first "smp_rmb();" in function
>>> unmap_common for the next iteration with the "block_speculation" macro.
>> I just checked this one more time. The maptrack_entry macro has been
>> extended with the array_index_nospec macro already, so that the
>> assignment to the map variable is in bound. Therefore, I actually will
>> not introduce the block_speculation macro.
> unmap_common() uses maptrack_entry() with op->handle. I didn't
> refer to that, because - as you say - maptrack_entry() is itself
> getting hardened already. Instead I am, as said, referring to
> map->ref / op->ref.
>
> And no, replacing _any_ smb_rmb() would not be correct: The
> barriers are needed unconditionally, whereas block_speculation()
> inserts a barrier only in a subset of cases (for example never on
> Arm).
Right. I will protect the index operations based on op->ref in
unmap_common via array_index_nospec.
>
>>> The other check unlikely(op->ref >= nr_grant_entries(rgt)) can only
>>> reach out-of-bounds for the unmap case, in case the map->ref entry has
>>> been out-of-bounds beforehand. I did not find an assignment that is not
>>> protected by a bound check and a speculation barrier or array_nospec_index.
> I can only refer you to the comment there again. In essence, the prior
> bounds check done may have been against the grant table limits of
> another domain. You may want to look at the full commit introducing this
> comment.

In unmap_common_complete, IHMO it is sufficient to evaluate the first
check op->done via evaluate_nospec, so that the return is taken in case
nothing has been done, and then invalid values of op->ref should not be
used under speculation, or out-of-bounds. On the other hand, this
function is always called after gnttab_flush_tlb. I did not spot a good
indicator for that function blocking speculation, hence, I would still
add the macro.

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel