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

[Andre Przywara <andre.przywara@xxxxxxx>]

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

Do I miss something here? It sounds a bit scary that we can dump this
guest memory access scheme which gave us so many headaches and kept us
busy for some months now ...

Cheers,
Andre.

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