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

[Jan Beulich <JBeulich@xxxxxxxx>]

On 10.07.2019 10:54, Norbert Manthey wrote:
> On 7/10/19 05:04, Jan Beulich wrote:
>> On 08.07.2019 14:58, Norbert Manthey wrote:
>>> On 5/24/19 13:10, Jan Beulich wrote:
>>>>>>> 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:
>>>>>>> --- 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).
>>> I agree, the given protection does not force the CPU to pick up the
>>> fixed value. As you already noticed, the presented fix might not work in
>>> all cases, but is among the suitable solutions we have today to prevent
>>> simple user controlled out-of-bound accesses during speculation. Relying
>>> on the stale value of the register that might be used during speculation
>>> makes a potential out-of-bound access much more difficult. Hence, the
>>> proposed solution looks good enough to me.
>> But using a local variable further reduces the risk afaict: Either
>> the compiler puts it into a register, in which case we're entirely
>> fine. Or it puts it on the stack, which I assume is more likely to
>> stay in cache than a reference to some other data structure (iirc
>> also on the stack, but not in the current frame).
> If you want me to introduce a local variable, I can do that. I remember
> we had discussions around that as well.

I think this would be (at least slightly) better, yes.

Jan

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