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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel