On 2/28/19 11:00, Jan Beulich wrote: >>>> On 27.02.19 at 14:01, <nmant...@amazon.de> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
