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


Xen-devel mailing list



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