On 2/7/19 15:00, Jan Beulich wrote: >>>> On 07.02.19 at 11:20, <nmant...@amazon.de> wrote: >> On 2/7/19 10:50, Norbert Manthey wrote: >>> On 2/6/19 16:53, Jan Beulich wrote: >>>>>>> On 06.02.19 at 16:06, <nmant...@amazon.de> wrote: >>>>> On 2/6/19 15:52, Jan Beulich wrote: >>>>>>>>> On 29.01.19 at 15:43, <nmant...@amazon.de> wrote: >>>>>>> @@ -963,6 +965,9 @@ map_grant_ref( >>>>>>> PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n", >>>>>>> op->ref, rgt->domain->domain_id); >>>>>>> >>>>>>> + /* Make sure the above check is not bypassed speculatively */ >>>>>>> + op->ref = array_index_nospec(op->ref, nr_grant_entries(rgt)); >>>>>>> + >>>>>>> act = active_entry_acquire(rgt, op->ref); >>>>>>> shah = shared_entry_header(rgt, op->ref); >>>>>>> status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, >>>>>>> op->ref); >>>>>> Just FTR - this is a case where the change, according to prior >>>>>> discussion, is pretty unlikely to help at all. The compiler will have >>>>>> a hard time realizing that it could keep the result in a register past >>>>>> the active_entry_acquire() invocation, as that - due to the spin >>>>>> lock acquired there - acts as a compiler barrier. And looking at >>>>>> generated code (gcc 8.2) confirms that there's a reload from the >>>>>> stack. >>>>> I could change this back to a prior version that protects each read >>>>> operation. >>>> That or use block_speculation() with a comment explaining why. >>>> >>>> Also - why are there no changes at all to the unmap_grant_ref() / >>>> unmap_and_replace() call paths? Note in particular the security >>>> related comment next to the bounds check of op->ref there. I've >>>> gone through earlier review rounds, but I couldn't find an indication >>>> that this might have been the result of review feedback. >>> You are right. I am not sure whether I had a fix placed there in the >>> beginning. I will replace the first "smp_rmb();" in function >>> unmap_common for the next iteration with the "block_speculation" macro. >> I just checked this one more time. The maptrack_entry macro has been >> extended with the array_index_nospec macro already, so that the >> assignment to the map variable is in bound. Therefore, I actually will >> not introduce the block_speculation macro. > unmap_common() uses maptrack_entry() with op->handle. I didn't > refer to that, because - as you say - maptrack_entry() is itself > getting hardened already. Instead I am, as said, referring to > map->ref / op->ref. > > And no, replacing _any_ smb_rmb() would not be correct: The > barriers are needed unconditionally, whereas block_speculation() > inserts a barrier only in a subset of cases (for example never on > Arm). Right. I will protect the index operations based on op->ref in unmap_common via array_index_nospec. > >>> The other check unlikely(op->ref >= nr_grant_entries(rgt)) can only >>> reach out-of-bounds for the unmap case, in case the map->ref entry has >>> been out-of-bounds beforehand. I did not find an assignment that is not >>> protected by a bound check and a speculation barrier or array_nospec_index. > I can only refer you to the comment there again. In essence, the prior > bounds check done may have been against the grant table limits of > another domain. You may want to look at the full commit introducing this > comment.
In unmap_common_complete, IHMO it is sufficient to evaluate the first check op->done via evaluate_nospec, so that the return is taken in case nothing has been done, and then invalid values of op->ref should not be used under speculation, or out-of-bounds. On the other hand, this function is always called after gnttab_flush_tlb. I did not spot a good indicator for that function blocking speculation, hence, I would still add the macro. Best, Norbert 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
