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

[Julien Grall <julien.grall@arm.com>]

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@lists.xen.org
https://lists.xen.org/xen-devel