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

[Jan Beulich <JBeulich@xxxxxxxx>]

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

>>>>> @@ -3863,6 +3883,9 @@ static int gnttab_get_status_frame_mfn(struct 
>>>>> domain *d,
>>>>>               return -EINVAL;
>>>>>       }
>>>>>   
>>>>> +    /* Make sure idx is bounded wrt nr_status_frames */
>>>>> +    block_speculation();
>>>>> +
>>>>>       *mfn = _mfn(virt_to_mfn(gt->status[idx]));
>>>>>       return 0;
>>>>>   }
>>>> Why don't you use array_index_nospec() here? And how come
>>> There is no specific reason. I will swap.
>>>> speculation into q() is fine a few lines above?
>>> I do not see a reason why it would be bad to enter that function
>>> speculatively. There are no accesses that would have to be protected by
>>> extra checks, afaict. Otherwise, that function should be protected by
>>> its own.
>> Which in fact happens, but only in patch 3. This may be worth saying
>> explicitly here.
> 
> Do you want me to explicitly mention this in the commit message, or add
> a comment here, which I have to drop in patch 3 again? For now, I'd just
> leave it as is, as the version based fixes are handled in the other commit.

A commit message remark would both help understand things now and
in the future, and at the same time avoid me or someone else re-
raising the question next time round, not the least because of the
noticable gaps between versions.

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