Re: [PATCH v4 1/2] KVM: arm64: vgic: Fix race between LPI release and re-registration

[Oliver Upton <oupton@kernel.org>]

On Fri, Jul 03, 2026 at 03:13:34PM -0700, Oliver Upton wrote:
> Hi Carlos,
> 
> Please wait a few days between spins, we were in the middle of a
> conversation and I didn't have a chance to reply to you since yesterday.
> 
> On Fri, Jul 03, 2026 at 02:26:00PM +0200, Carlos López wrote:
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 67d107e9a77d..577286069368 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -116,7 +116,15 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
> >  		kfree(irq);
> >  		irq = oldirq;
> >  	} else {
> > -		ret = xa_err(__xa_store(&dist->lpi_xa, intid, irq, 0));
> > +		/*
> > +		 * The entry is either empty or contains a dead LPI (refcount=0)
> > +		 * from the deferred release path, pending cleanup by
> > +		 * vgic_release_deleted_lpis(). Evict and free it if present.
> > +		 */
> > +		oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0);
> > +		ret = xa_err(oldirq);
> > +		if (!ret && oldirq)
> > +			kfree_rcu(oldirq, rcu);
> 
> We need to assert the assumption here that the evicted IRQ has a nonzero

s/nonzero/zero

Obviously not reading what the hell I'm writing :)

> refcount and is pending release. Otherwise I feel like possibly leaking
> memory is better than potentially introducing a UAF.
> 
> Untested diff below.
> 
> Thanks,
> Oliver
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 4477f870c7b3..621a83399184 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -116,18 +116,28 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
>  		kfree(irq);
>  		irq = oldirq;
>  	} else {
> -		ret = xa_err(__xa_store(&dist->lpi_xa, intid, irq, 0));
> -	}
> -
> -	xa_unlock_irqrestore(&dist->lpi_xa, flags);
> +		oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0);
> +		ret = xa_err(oldirq);
> +		if (ret) {
> +			xa_unlock_irqrestore(&dist->lpi_xa, flags);
> +			kfree(irq);
>  
> -	if (ret) {
> -		xa_release(&dist->lpi_xa, intid);
> -		kfree(irq);
> +			return ERR_PTR(ret);
> +		}
>  
> -		return ERR_PTR(ret);
> +		/*
> +		 * The expectation is that only LPIs which are pending release
> +		 * could possibly be evicted by storing the new entry. Free the
> +		 * old IRQ only if this is the case, as otherwise someone else
> +		 * could still hold a pointer on the outgoing IRQ.
> +		 */
> +		if (oldirq && !WARN_ON_ONCE(refcount_read(&oldirq->refcount) &&
> +					    oldirq->pending_release))

		if (oldirq && !WARN_ON_ONCE(refcount_read(&oldirq->refcount) ||
					    !oldirq->pending_release))

Thanks,
Oliver