On 7/11/19 14:34, Jan Beulich wrote: > 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. True, but a rebooted domain might come up on a different core, which results in using different hardware structures. The "repeatedly" is open to be interpreted, I agree. From my understanding, it belongs to the properties to have to reliably target a speculative out-of-bound access. > >> @@ -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. I will move it accordingly, however, as discussed below, I'll drop this one instead. > >> @@ -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? I wanted to make the two as close as possible, I will change it back as I did not find a difference in the binary. > > 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. I wanted to make the use of the new variable as close as possible to the bound check and instruction blocking speculation. > >> - 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. It looks fine to me, but I'll double check. > >> @@ -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.
I'll wrap the lines. 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
