[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v9 19/28] ARM: vITS: handle MAPD command

Hi Andre,

On 05/24/2017 10:10 AM, Andre Przywara wrote:
On 17/05/17 19:07, Julien Grall wrote:
  * Lookup the address of the Interrupt Translation Table associated with
  * that device ID.
@@ -414,6 +429,133 @@ out_unlock:
     return ret;

+/* Must be called with the ITS lock held. */
+static int its_discard_event(struct virt_its *its,
+                             uint32_t vdevid, uint32_t vevid)
+    struct pending_irq *p;
+    unsigned long flags;
+    struct vcpu *vcpu;
+    uint32_t vlpi;
+    ASSERT(spin_is_locked(&its->its_lock));
+    if ( !read_itte_locked(its, vdevid, vevid, &vcpu, &vlpi) )
+        return -ENOENT;
+    if ( vlpi == INVALID_LPI )
+        return -ENOENT;
+    /* Lock this VCPU's VGIC to make sure nobody is using the
pending_irq. */
+    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);

There is an interesting issue happening with this code. You don't check
the content of the memory provided by the guest. So a malicious guest
could craft the memory in order to setup mapping with known vlpi and a
different vCPU.

This would lead to use the wrong lock here and corrupt the list.

What about this:
Right now (mostly due to the requirements of the INVALL implementation)
we store the VCPU ID in our struct pending_irq, populated upon MAPTI. So
originally this was just for caching (INVALL being the only user of
this), but I was wondering if we should move the actual instance of this
information to pending_irq instead of relying on the collection ID from
the ITS table. So we would never need to look up and trust the ITS
tables for this information anymore. Later with the VGIC rework we will
need this field anyway (even for SPIs).

I think this should solve this threat, where a guest can manipulate Xen
by crafting the tables. Tinkering with the other information stored in
the tables should not harm Xen, the guest would just shoot itself into
the foot.

Does that make sense?

I think so. If I understand correctly, with that solution we would not need to protect the memory provided by the guest?


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.