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

[Oliver Upton <oupton@kernel.org>]

Hi Carlos,

Thanks for reporting this ugly bug.

On Fri, Jul 03, 2026 at 04:15:08AM +0200, Carlos López wrote:
> To fix the direct release path, move the reference count drop inside
> the xarray lock, making sure that vgic_add_lpi() never encounters the
> to-be-released LPI.

As Sashiko pointed out, this is going to massively regress performance
of LPI injection. I don't think this is going to be a viable option.

> To fix the deferred release path, since the refcount drop must happen
> under a raw spinlock, the same solution does not work. Instead, update
> vgic_add_lpi(), so that if it evicts a non-NULL refcount=0 LPI from the
> xarray, it takes on the responsibility of releasing it. If this happens,
> vgic_release_deleted_lpis() will iterate the xarray normally and will
> simply not find the already released structure.
> 
> Reported-by: Claude:claude-opus-4-6
> Fixes: 3a08a6ca7c37 ("KVM: arm64: vgic-v3: Use bare refcount for VGIC LPIs")
> Fixes: d54594accf73 ("KVM: arm64: vgic-v3: Erase LPIs from xarray outside of raw spinlocks")
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
> v2:
> * Address Sashiko's review. Fix the direct release path by decrementing the
>   refcount under the xarray spinlock, preventing a UAF that would have been
>   introduced in v1.

So I actually agree with your approach in v1, vgic_release_lpi_locked()
should do an __xa_cmpxchg() to only erase if the to-be-deleted IRQ that
it owns remains in the xarray.

I believe the UAF could've been avoided by unconditionally calling
kfree_rcu() in vgic_release_lpi_locked() and not attempting to cleanup
dead LPIs in vgic_add_lpi(). IOW, whoever takes the refcount of an LPI
to 0 always has the responsibility of freeing it.

Maybe below would be enough?

Thanks,
Oliver

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 5a4768d8cd4f..4c79e1096af4 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -132,7 +132,14 @@ 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)
 {
 	lockdep_assert_held(&dist->lpi_xa.xa_lock);
-	__xa_erase(&dist->lpi_xa, irq->intid);
+
+	/*
+	 * Another LPI could've been inserted prior to taking the xa_lock, as
+	 * vgic_add_lpi() can only take a reference on a pre-existing LPI if
+	 * the refcount is nonzero. While freeing the object is always done here,
+	 * only delete the entry @INTID if it is this IRQ.
+	 */
+	__xa_cmpxchg(&dist->lpi_xa, irq->intid, irq, NULL, 0);
 	kfree_rcu(irq, rcu);
 }