Re: [Xen-devel] [PATCH L1TF v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 2/28/19 11:00, Jan Beulich wrote:
>>>> On 27.02.19 at 14:01, <nmanthey@xxxxxxxxx> wrote:
>> On 2/25/19 17:46, Jan Beulich wrote:
>>> I would really like to ask that I (or someone else) don't need to
>>> go through and list remaining version checks again - after all I
>>> had done so for v6 already, and I didn't go through all of them
>>> again for v7 assuming that you would have worked through the
>>> entire set.
>> So, here is the annotation for all of them. Anyone that I did not
>> include in the list has been fixed in previous versions, or will be
>> fixed in the next version:
>>
>> git grep -np version common/grant_table.c
>>
>> common/grant_table.c:831:static int _set_status(unsigned gt_version,
>> common/grant_table.c:840:    if ( gt_version == 1 )
>>
>> -> I do not see how out-of-bound accesses happen in the called functions
>> there.
> Both functions get shah passed into them, which may point to the
> other version's layout. Earlier fences don't help speculation on this
> conditional.
Whenever this function is called, the shah field has the same structure
for both versions. The v2 grant entry has a plain header hdr, which is
used here, and which is forwarded to the set_status method. Another
property that we never talked about here is that we only care about
cross cache-line accesses. As soon as any byte is touched on a cache
line, the whole line is brought into the cache. As the code around the
set_status method already pulls in the corresponding cache line, that
code looks okay to me from a L1TF perspective.
>
>> common/grant_table.c=1444=unmap_common_complete(struct
>> gnttab_unmap_common *op)
>> common/grant_table.c:1469:    if ( rgt->gt_version == 1 )
>>
>> -> I do not see how to be exploitable, as the shared_entry_header call
>> above just used an lfence.
> Again, earlier fences don't suppress subsequent speculation on
> an independent conditional.

Yes, agreed. However, in the current version, the array is allocated
already for the one branch, and the flags are in the cache for both
versions as well. The above evaluate_nospec that protects the evaluation
of the variable "done" above prevents out-of-bound values for ref. Now,
if you want me to be prepared for the future where people might move the
allocation of the status array from the generic code to the v2 specific
code, I would have to add another lfence here today already.

>
>> common/grant_table.c=1761=gnttab_grow_table(struct domain *d, unsigned
>> int req_nr_frames)
>> common/grant_table.c:1800:    /* Status pages - version 2 */
>> common/grant_table.c:1801:    if ( gt->gt_version > 1 )
>>
>> -> I do not see how out-of-bound access could happen. This calls
>> gnttab_populate_status_frames that allocates pages and should not touch
>> more memory than before
> We've been talking about the speculation window being perhaps
> hundreds of insns. It may be purely theoretical, but speculation all
> the way through alloc_xenheap_page() would lead to a speculative
> store to gt->status[]. _Right now_ that array gets allocated in
> grant_table_init(), but I can't see why that couldn't be moved to
> gnttab_set_version(), so the access above could be a latent issue.

I agree that this might be a latent issue, and in case people modify the
code, the effect wrt L1TF mitigation might have to be taken into
account. While we have a specific problematic code snippet here, that
statement is true for all the other mitigations as well. Since I do not
see a problem with the code today, and I do not like to pay the penalty
of the lfence to protect against something that might not happen in the
future, I would not add the lfence here.

Best,
Norbert

>
> I'll skip going into details of further ones, assuming that some of
> them follow patterns above.
>
> Jan
>
>




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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