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