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. > > 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. > > 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
