>>> 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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
