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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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