On 10.07.2019 10:54, Norbert Manthey wrote: > On 7/10/19 05:04, Jan Beulich wrote: >> On 08.07.2019 14:58, Norbert Manthey wrote: >>> On 5/24/19 13:10, Jan Beulich wrote: >>>>>>> On 24.05.19 at 11:54, <nmant...@amazon.de> wrote: >>>>> On 5/23/19 16:17, Jan Beulich wrote: >>>>>>>>> On 21.05.19 at 09:45, <nmant...@amazon.de> wrote: >>>>>>> --- 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. >>>> I don't buy this argumentation: In particular if the cache line got >>>> flushed for whatever reason, the load may take almost arbitrarily >>>> long, opening up a large speculation window again using the >>>> destination register of the load (whatever - not bounds checked - >>>> value that ends up holding). >>> I agree, the given protection does not force the CPU to pick up the >>> fixed value. As you already noticed, the presented fix might not work in >>> all cases, but is among the suitable solutions we have today to prevent >>> simple user controlled out-of-bound accesses during speculation. Relying >>> on the stale value of the register that might be used during speculation >>> makes a potential out-of-bound access much more difficult. Hence, the >>> proposed solution looks good enough to me. >> But using a local variable further reduces the risk afaict: Either >> the compiler puts it into a register, in which case we're entirely >> fine. Or it puts it on the stack, which I assume is more likely to >> stay in cache than a reference to some other data structure (iirc >> also on the stack, but not in the current frame). > If you want me to introduce a local variable, I can do that. I remember > we had discussions around that as well.
I think this would be (at least slightly) better, yes. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
