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. > >>>>>> @@ -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 q() 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. >>> Which in fact happens, but only in patch 3. This may be worth saying >>> explicitly here. >> Do you want me to explicitly mention this in the commit message, or add >> a comment here, which I have to drop in patch 3 again? For now, I'd just >> leave it as is, as the version based fixes are handled in the other commit. > A commit message remark would both help understand things now and > in the future, and at the same time avoid me or someone else re- > raising the question next time round, not the least because of the > noticable gaps between versions.
I will extend the commit message accordingly. 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
