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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] The patch introduces a Use-After-Free (UAF) and ABA vulnerability by allowing `vgic_add_lpi()` to free an LPI via `kfree_rcu()` while a concurrent `vgic_put_irq()` could still be processing the same LPI's release without RCU protection.

Pre-existing issues:
- [High] `vgic_add_lpi()` can spuriously fail with `-ENOMEM` when racing with an LPI release due to a lost XArray node reservation.
--

commit 3cbf900c0e9154b2814ac964d591008312ad60be
Author: Carlos López <clopez@suse.de>

KVM: arm64: vgic: Fix race between LPI release and re-registration

Fix a potential race between decrementing an LPI's reference count and
evicting that structure from the LPI xarray.

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> --- 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 an IRQ whose refcount is 0
> +		 * but has not yet been freed by vgic_release_lpi_locked(). Evict the
> +		 * IRQ and free it, if there is one.
> +		 */
> +		oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0);

[Severity: High]
This is a pre-existing issue, but can this __xa_store() fail with -ENOMEM
when racing with an LPI release?

Looking earlier in vgic_add_lpi(), xa_reserve_irq() allocates an XArray
node, but the lock is released immediately after. If a concurrent
vgic_release_lpi_locked() replaces the entry with NULL (or erases it before
this patch), the underlying XArray node might be freed.

When this thread resumes and calls __xa_store() here with gfp=0, could it
fail due to the missing node and cause the guest ITS command to be dropped?

> +		ret = xa_err(oldirq);
> +		if (!ret && oldirq)
> +			kfree_rcu(oldirq, rcu);
>  	}
>  
>  	xa_unlock_irqrestore(&dist->lpi_xa, flags);

[ ... ]

> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -131,9 +131,19 @@ struct vgic_irq *vgic_get_vcpu_irq(struct kvm_vcpu *vcpu, u32 intid)
>  
>  static void vgic_release_lpi_locked(struct vgic_dist *dist, struct vgic_irq *irq)
>  {
> +	struct vgic_irq *old;
>  
>  	lockdep_assert_held(&dist->lpi_xa.xa_lock);
> -	__xa_erase(&dist->lpi_xa, irq->intid);
> -	kfree_rcu(irq, rcu);
> +
> +	/*
> +	 * Free the IRQ if it is still present in the xarray, as its entry could have
> +	 * been overwritten after the refcount was dropped, but before the xarray lock
> +	 * was acquired. If the cmpxchg fails, vgic_add_lpi() grabbed the lock first
> +	 * and freed the old IRQ along the way.
> +	 */
> +	old = __xa_cmpxchg(&dist->lpi_xa, irq->intid, irq, NULL, 0);

[Severity: Critical]
Does this introduce a use-after-free and ABA issue if the RCU grace period
completes before this thread acquires the xarray lock?

When vgic_put_irq() drops the refcount to 0, the thread does not hold an RCU
read lock while waiting to acquire the xarray lock. A concurrent
vgic_add_lpi() could replace the old LPI and call kfree_rcu(oldirq, rcu).

Because the releasing thread isn't in an RCU critical section, the grace
period can elapse. The memory backing this irq can then be freed and
reallocated to a new LPI with the same intid.

When this thread eventually resumes and calls __xa_cmpxchg(), wouldn't it
dereference the freed irq pointer to read irq->intid, and then incorrectly
match the reallocated LPI, causing a double-free?

> +	if (old == irq)
> +		kfree_rcu(irq, rcu);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702233514.2581921-1-clopez@suse.de?part=1