On 5/23/19 17:01, Jan Beulich wrote: >>>> On 21.05.19 at 09:45, <nmant...@amazon.de> wrote: >> Guests can issue grant table operations and provide guest controlled >> data to them. This data is used as index for memory loads after bound >> checks have been done. Depending on the grant table version, the >> size of elements in containers differ. As the base data structure is >> a page, the number of elements per page also differs. Consequently, >> bound checks are version dependent, so that speculative execution can >> happen in several stages, the bound check as well as the version check. >> >> This commit mitigates cases where out-of-bound accesses could happen >> due to the version comparison. In cases, where no different memory >> locations are accessed on the code path that follow an if statement, >> no protection is required. No different memory locations are accessed >> in the following functions after a version check: >> >> * gnttab_setup_table: only calculated numbersi are used, and then >> function gnttab_grow_table is called, which is version protected >> >> * gnttab_transfer: the case that depends on the version check just gets >> into copying a page or not > Well, this is a little lax, but I'm willing to accept it. There could, after > all, still be speculation into alloc_domheap_page(). > >> * acquire_grant_for_copy: the not fixed comparison is on the abort path >> and does not access other structures, and on the else branch only >> accesses structures that are properly allocated > As said in my recent reply to v10 of the original series, in particular > for fixup_status_for_copy_pin() this isn't immediately obvious. In > no case is "does not access other structures" true, though. How > about saying "accesses only structures that have been validated > before" or some such instead (I don't particularly like "validated" > here, but I can't currently think of anything better)? I will rephrase accordingly. > >> * gnttab_set_version: all accessible data is allocated for both versions > This is not enough for my taste: The very first loop is safe only > because nr_grant_entries() is. And speculating into > gnttab_unpopulate_status_frames() doesn't look safe at all, as > gt->status[i] may be NULL. So, you basically want to see a block_speculation() at the beginning of the function gnttab_populate_status_frames and gnttab_unpopulate_status_frames? I do not claim to protect against speculative out-of-bound accesses that are caused by the for loop in gnttab_set_version. > >> * gnttab_release_mappings: this function is called only during domain >> destruction and control is not returned to the guest >> >> * mem_sharing_gref_to_gfn: speculation will be stoped by the second if >> statement, as that places a barrier on any path to be executed. >> >> * gnttab_get_status_frame_mfn: no version dependent check, because all >> accesses, except the gt->status[idx], do not perform actual >> out-of-bound accesses, including the gnttab_grow_table function >> call. > Nit: I very much hope no code anywhere performs _actual_ out of > bound accesses. I'm sure you mean speculative ones here. Yes, I do not mean actual out-of-bound accesses. What I actually meant: all other accesses in this function are to variables, and not based on an index. > >> * gnttab_get_shared_frame: block_speculation in >> gnttab_get_status_frame_mfn blocks accesses > The former doesn't call the latter, and as per my patch 2 comments > gnttab_get_shared_frame_mfn() looks to remain unprotected after > patch 2.
True, I will add a block_speculation() below the assert statement, so that the condition is true even when executing speculatively. Best, Norbert Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
