Re: [Xen-devel] [PATCH v6 10/36] ARM: GIC: Add checks for NULL pointer pending_irq's

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 07/04/17 18:32, Andre Przywara wrote:
> For LPIs the struct pending_irq's are somewhat dynamically allocated and
When I read "dynamically", I directly ask myself. What is protecting the 
pending_irq structure to be freed whilst in-use in the vgic code?
In the current design, this would happen if a device is removed from a 
domain (e.g via the MAPD command for DOM0).
I cannot find any code which would prevent that and I think DOM0 can 
take down Xen by mistake. Imagine a pending IRQ whilst executing MAPD(V=0).
So what's the plan?

> the pointers are stored in a radix tree. While I convinced myself that
> an invalid LPI number can't make it into the core code, people might be
> concerned about NULL pointer dereferences.
> So add checks in some places just to be on the safe side.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c  | 23 +++++++++++++++++++++++
>  xen/arch/arm/vgic.c |  4 ++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index da19130..44c34b1 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -405,6 +405,13 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int 
> virtual_irq)
>      struct pending_irq *p = irq_to_pending(v, virtual_irq);
>      unsigned long flags;
>
> +    /*
> +     * If an LPIs has been removed meanwhile, it has been cleaned up
> +     * already, so nothing to remove here.
> +     */
> +    if ( !p )
> +        return;
> +
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>      if ( !list_empty(&p->lr_queue) )
>          list_del_init(&p->lr_queue);
> @@ -415,6 +422,10 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int 
> virtual_irq)
>  {
>      struct pending_irq *n = irq_to_pending(v, virtual_irq);
>
> +    /* If an LPI has been removed meanwhile, there is nothing left to raise. */
> +    if ( !n )
> +        return;
> +
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>
>      if ( list_empty(&n->lr_queue) )
> @@ -461,7 +472,19 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>
>      gic_hw_ops->read_lr(i, &lr_val);
>      irq = lr_val.virq;
> +
Please don't make spurious change...

>      p = irq_to_pending(v, irq);
> +    /* An LPI might have been unmapped, in which case we just clean up here. */
> +    if ( !p )
> +    {
> +        ASSERT(is_lpi(irq));
> +
> +        gic_hw_ops->clear_lr(i);
> +        clear_bit(i, &this_cpu(lr_mask));
> +
> +        return;
> +    }
> +
>      if ( lr_val.state & GICH_LR_ACTIVE )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 83569b0..b7ee105 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -470,6 +470,10 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> virq)
>      unsigned long flags;
>      bool running;
>
> +    /* If an LPI has been removed, there is nothing to inject here. */
> +    if ( !n )
> +        return;
> +
>      priority = vgic_get_virq_priority(v, virq);
>
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
Cheers,

--
Julien Grall

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