On 10.07.2019 14:54, Norbert Manthey 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. To avoid speculative out-of-bound accesses, we > use the array_index_nospec macro where applicable, or the macro > block_speculation. Note, the block_speculation macro is used on all > path in shared_entry_header and nr_grant_entries. This way, after a > call to such a function, all bound checks that happened before become > architectural visible, so that no additional protection is required > for corresponding array accesses. As the way we introduce an lfence > instruction might allow the compiler to reload certain values from > memory multiple times, we try to avoid speculatively continuing > execution with stale register data by moving relevant data into > function local variables. > > Speculative execution is not blocked in case one of the following > properties is true: > - path cannot be triggered by the guest > - path does not return to the guest > - path does not result in an out-of-bound access > - path cannot be executed repeatedly
Upon re-reading I think this last item isn't fully applicable: I think you attach such an attribute to domain creation/destruction functions. Those aren't executed repeatedly for a single guest, but e.g. rapid rebooting of a guest (or several ones) may actually be able to train such paths. > @@ -2091,6 +2100,7 @@ gnttab_transfer( > struct page_info *page; > int i; > struct gnttab_transfer gop; > + grant_ref_t ref; This declaration would better live in the more narrow scope it's (only) used in. > @@ -2237,8 +2247,14 @@ gnttab_transfer( > */ > spin_unlock(&e->page_alloc_lock); > okay = gnttab_prepare_for_transfer(e, d, gop.ref); > + ref = gop.ref; Other than in the earlier cases here you copy a variable that's already local to the function. Is this really helpful? Independent of this - is there a particular reason you latch the value into the (second) local variable only after its first use? It likely won't matter much, but it's a little puzzling nevertheless. > - if ( unlikely(!okay || assign_pages(e, page, 0, MEMF_no_refcount)) ) > + /* > + * Make sure the reference bound check in gnttab_prepare_for_transfer > + * is respected and speculative execution is blocked accordingly > + */ > + if ( unlikely(!evaluate_nospec(okay)) || > + unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) ) If I can trust my mail UI (which I'm not sure I can) there's an indentation issue here. > @@ -3853,7 +3879,8 @@ static int gnttab_get_status_frame_mfn(struct domain *d, > return -EINVAL; > } > > - *mfn = _mfn(virt_to_mfn(gt->status[idx])); > + /* Make sure idx is bounded wrt nr_status_frames */ > + *mfn = _mfn(virt_to_mfn(gt->status[array_index_nospec(idx, > nr_status_frames(gt))])); This and ... > @@ -3882,7 +3909,8 @@ static int gnttab_get_shared_frame_mfn(struct domain *d, > return -EINVAL; > } > > - *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx])); > + /* Make sure idx is bounded wrt nr_status_frames */ > + *mfn = _mfn(virt_to_mfn(gt->shared_raw[array_index_nospec(idx, > nr_grant_frames(gt))])); ... this line are too long now and hence need wrapping. Jan _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
