[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 07.02.19 at 11:20, <nmanthey@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>>>> On 2/6/19 15:52, Jan Beulich wrote:
>>>>>>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> 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).

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

Jan



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.