On Thu, 25 May 2017, Julien Grall wrote: > On 25/05/2017 19:55, Stefano Stabellini wrote: > > On Wed, 24 May 2017, Andre Przywara wrote: > > > Hi, > > > > > > On 24/05/17 10:56, Julien Grall wrote: > > > > 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? > > > > > > Well, it gets better (though also a bit scary): > > > Currently we use the guest's ITS tables to translate a DeviceID/EventID > > > pair to a vLPI/vCPU pair. Now there is this new > > > gicv3_its_get_event_pending_irq() function, which also takes an ITS and > > > an DeviceID/EventID pair and gives us a struct pending_irq. > > > And here we have both the vLPI number and the VCPU number in there > > > already, so actually we don't need read_itte() anymore. And if we don't > > > read, we don't need write. And if we don't write, we don't need to > > > access guest memory. So this seems to ripple through and allows us to > > > possibly dump the guest memory tables altogether. > > > > Sounds like a good idea to me for DeviceID/EventID to vLPI/vCPU > > translations. > > > > > > > Now we still use the collection table in guest memory, but I was > > > wondering if we could store the collection ID in the vcpu struct and use > > > some hashing scheme to do the reverse lookup. But that might be > > > something for some future cleanup / optimization series. > > > > Leaving the security angle aside for a moment, I would prefer to keep > > the guest memory accesses rather than adding another hashing scheme to > > Xen for collection IDs. > > The spec only require you to implement max cpus + 1 collections. I don't think > an hashing scheme would be necessary here. It is a simple array (1 byte per > entry in today). > > > > > Going back to security: it looks like it should be possible to check for > > the validity of collection IDs without too much troubles? > > too? > > If we store everthing in the pending_irq the use of the collection would > limited to a few commands (e.g MOVI, MAPTI...). We don't much care if the > guest modify the collection table as long as we check the vCPU is valid.
That's what I thought. In that case we might as well keep the info in guest memory. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel