On 5/23/19 16:17, 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. To avoid speculative out-of-bound accesses, we >> use the array_index_nospec macro where applicable, or the macro >> block_speculation. Note, that the block_speculation is always used in > s/always/already/ ? They both work, but the 'always' underlines that a caller can rely on the fact that this will happen on all execution path of that function. Hence, I like to stick to 'always' here. > >> the calls to shared_entry_header and nr_grant_entries, so that no >> additional protection is required once these functions have been >> called. > Isn't this too broad a statement? There's some protection, but not > for just anything that follows. You are right, to given protection is that any bound check that could have been bypassed speculatively is enforced after calling one of the two functions. I will rephrase the commit message accordingly. > >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -988,9 +988,10 @@ map_grant_ref( >> PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n", >> op->ref, rgt->domain->domain_id); >> >> - act = active_entry_acquire(rgt, op->ref); >> + /* This call also ensures the above check cannot be passed >> speculatively */ >> shah = shared_entry_header(rgt, op->ref); >> status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, >> op->ref); >> + act = active_entry_acquire(rgt, op->ref); > I know we've been there before, but what guarantees that the > compiler won't reload op->ref from memory for either of the > latter two accesses? In fact afaict it always will, due to the > memory clobber in alternative(). The compiler can reload op->ref from memory, that is fine here, as the bound check happens above, and the shared_entry call comes with an lfence() by now, so that we will not continue executing speculatively with op->ref being out-of-bounds, independently of whether it's from memory or registers. > >> @@ -3863,6 +3883,9 @@ static int gnttab_get_status_frame_mfn(struct domain >> *d, >> return -EINVAL; >> } >> >> + /* Make sure idx is bounded wrt nr_status_frames */ >> + block_speculation(); >> + >> *mfn = _mfn(virt_to_mfn(gt->status[idx])); >> return 0; >> } > Why don't you use array_index_nospec() here? And how come There is no specific reason. I will swap. > speculation into gnttab_grow_table() is fine a few lines above? I do not see a reason why it would be bad to enter that function speculatively. There are no accesses that would have to be protected by extra checks, afaict. Otherwise, that function should be protected by its own. > And what about the similar code in gnttab_get_shared_frame_mfn()? I will add an array_nospec_index there as well. > > 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
