Re: [PATCH v8 15/27] ARM: vITS: handle CLEAR command

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

On 12/04/17 15:29, Andre Przywara wrote:
> Hi,
>
> On 12/04/17 15:10, Julien Grall wrote:
>> Hi Andre,
>>
>> On 12/04/17 01:44, Andre Przywara wrote:
>>> This introduces the ITS command handler for the CLEAR command, which
>>> clears the pending state of an LPI.
>>> This removes a not-yet injected, but already queued IRQ from a VCPU.
>>> As read_itte() is now eventually used, we add the static keyword.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  xen/arch/arm/vgic-v3-its.c | 52
>>> ++++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>>> index 632ab84..e24ab60 100644
>>> --- a/xen/arch/arm/vgic-v3-its.c
>>> +++ b/xen/arch/arm/vgic-v3-its.c
>>> @@ -227,8 +227,8 @@ static bool read_itte_locked(struct virt_its *its,
>>> uint32_t devid,
>>>   * This function takes care of the locking by taking the its_lock
>>> itself, so
>>>   * a caller shall not hold this. Before returning, the lock is
>>> dropped again.
>>>   */
>>> -bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
>>> -               struct vcpu **vcpu_ptr, uint32_t *vlpi_ptr)
>>> +static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t
>>> evid,
>>> +                      struct vcpu **vcpu_ptr, uint32_t *vlpi_ptr)
>>>  {
>>>      bool ret;
>>>
>>> @@ -297,6 +297,51 @@ static uint64_t its_cmd_mask_field(uint64_t
>>> *its_cmd, unsigned int word,
>>>  #define its_cmd_get_validbit(cmd)       its_cmd_mask_field(cmd, 2,
>>> 63,  1)
>>>  #define its_cmd_get_ittaddr(cmd)        (its_cmd_mask_field(cmd, 2,
>>> 8, 44) << 8)
>>>
>>> +/*
>>> + * CLEAR removes the pending state from an LPI. */
>>> +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr)
>>> +{
>>> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
>>> +    uint32_t eventid = its_cmd_get_id(cmdptr);
>>> +    struct pending_irq *p;
>>> +    struct vcpu *vcpu;
>>> +    uint32_t vlpi;
>>> +    unsigned long flags;
>>> +
>>> +    /* Translate the DevID/EvID pair into a vCPU/vLPI pair. */
>>> +    if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) )
>>
>> So the vCPU ID is retrieved from guest memory, therefore you cannot
>> trust the value for taking a lock.
>>
>> Indeed, a malicious guest could rewrite the value with another vCPU ID
>> and you would take the wrong vCPU vGIC lock.
>>
>> What is the plan to fix that?
>
> To (write-)protect the tables. I will mention that in the cover letter.
>
>>
>>> +        return -1;
>>> +
>>> +    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
>>
>> I don't think this lock will protect correctly the pending_irq because
>> if you do a MOVI before hand you will use the new vCPU ID and not the
>> old one (see my explanation on patch #2).
>>
>>> +
>>> +    p = its->d->arch.vgic.handler->lpi_to_pending(its->d, vlpi);
>>> +    if ( !p )
>>
>> Please detail what means NULL here.
>>
>>> +    {
>>> +        spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
>>> +        return -1;
>>> +    }
>>> +
>>> +    /*
>>> +     * If the LPI is already visible on the guest, it is too late to
>>> +     * clear the pending state. However this is a benign race that can
>>> +     * happen on real hardware, too: If the LPI has already been
>>> forwarded
>>> +     * to a CPU interface, a CLEAR request reaching the redistributor
>>> has
>>> +     * no effect on that LPI anymore. Since LPIs are edge triggered and
>>> +     * have no active state, we don't need to care about this here.
>>> +     */
>>> +    if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>>> +    {
>>> +        /* Remove a pending, but not yet injected guest IRQ. */
>>> +        clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
>>> +        list_del_init(&p->inflight);
>>> +        list_del_init(&p->lr_queue);
>>
>> I am not sure to understand why you open-code gic_remove_from_queues
>> rather than reworking it.
>
> Well, actually all that gic_remove_from_queues does is:
> 	 list_del_init(&p->lr_queue);
> under the VGIC lock with getting the pending_irq first.
> I have the struct pending_irq already, also the lock. So there is no
> point in calling that function (which would block anyway).
>
> And since I wanted to keep changes to the existing code to a minimum, I
> decided to just not touch this function, instead just put that *one*
> line in here, next to the other list_del_init().
> Does that sound sensible?

I am sorry but it does not. If one day someone decides to update 
gic_remove_from_queues, he will have to remember that we open-coded in 
MOVI because you didn't want to touch common code.

Common code is not set in stone, the goal is to abstract all the issues 
to make easier to propagate change. I am always in favor of reworking 
the common code.

Cheers,

-- 
Julien Grall

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