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

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

[Carlos López]

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

LPI structures are maintained in the VGIC LPI xarray (dist->lpi_xa).
When the reference count of an LPI structure drops to zero,
vgic_release_lpi_locked() removes the structure from the xarray and
frees it under the xarray lock.

However, the release of an LPI can race with a concurrent LPI
re-registration with the same INTID via vgic_add_lpi() on another CPU,
since the reference count drop and the xarray eviction are not performed
in a single atomic step. This can happen e.g. if the guest issues a
DISCARD while the LPI is still referenced from a vCPU's active-pending
list (ap_list), and the same INTID is re-mapped via MAPTI.

Particularly, vgic_release_lpi_locked() is called from two distinct
paths: direct release via vgic_put_irq(), and deferred release via
vgic_release_deleted_lpis(). During direct release, the issue can result
in deleting a newly registered LPI from the xarray:

  CPU0 (Releasing LPI)                    CPU1 (Adding new LPI)
  ====================                    =====================
  vgic_put_irq()
      __vgic_put_irq()
          refcount_dec_and_test()
                                          vgic_add_lpi()
                                              xa_lock_irqsave(..);
                                              old_irq = xa_load(&dist->lpi_xa, intid);
                                              vgic_try_get_irq_ref(old_irq) == false
                        new IRQ inserted -->  __xa_store(&dist->lpi_xa, intid, ..)
                                              xa_unlock_irqrestore(..);
  xa_lock_irqsave(..);
  vgic_release_lpi_locked()
      __xa_erase(&dist->lpi_xa, irq->intid);   <-- BUG: new IRQ is erased
      kfree_rcu(old_irq)

During the deferred release path, the old IRQ can be leaked:

  CPU0 (Releasing LPI)                    CPU1 (Adding new LPI)
  ====================                    =====================
  vgic_put_irq_norelease()
      __vgic_put_irq()
          refcount_dec_and_test()
      irq->pending_release = true
                                          vgic_add_lpi()
                                              xa_lock_irqsave(..);
                                              old_irq = xa_load(&dist->lpi_xa, intid);
                                              vgic_try_get_irq_ref() == false
                 BUG: old IRQ overwritten --> __xa_store(&dist->lpi_xa, intid, ..)
                                              xa_unlock_irqrestore(..);

  vgic_release_deleted_lpis()
      xa_lock_irqsave(..);
      xa_for_each() { .. } <-- old IRQ with pending_release = true
                               is gone, so it cannot be released

Fix both issues with two coordinated changes. On the release path,
erase and free the IRQ only if the xarray entry still contains the IRQ
being released. On the registration path, if a dead LPI is encountered
(non-NULL entry with refcount=0), release the old structure after
storing the new one. This ensures that whichever CPU acquires the
xarray lock first, it will take on the responsibility of releasing a
to-be-cleaned-up LPI.

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>
---
 arch/arm64/kvm/vgic/vgic-its.c | 10 +++++++++-
 arch/arm64/kvm/vgic/vgic.c     | 14 ++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 67d107e9a77d..42983d5211cc 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 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);
+		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
index 5a4768d8cd4f..b7f6b6c4dcf2 100644
--- 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);
+	if (old == irq)
+		kfree_rcu(irq, rcu);
 }
 
 static __must_check bool __vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)

base-commit: 1ee27dacbe5dc4def481794d899d67b0d4570094
-- 
2.51.0

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

[sashiko-bot]

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

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

[Carlos López]

On 7/3/26 1:52 AM, sashiko-bot@kernel.org wrote:
> 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.

kzalloc_obj() is called before kfree_rcu() in the winning CPU (running
vgic_add_lpi()), so the allocator cannot return back the same pointer
for the ABA issue to happen. This would need another DISCARD + MAPTI
cycle in parallel it to happen, which I guess is not impossible.

> 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?

Ugh, yes. On the vgic_put_irq() path the reference count needs to be
decremented under the xarray lock, that way there is no window where the
irq is in the xarray with refcount=0, so vgic_add_lpi() cannot free it
while we still hold the pointer to it.

For the deferred path I think it's okay though. Once
vgic_release_deleted_lpis() grabs the xarray lock, if the entry was
evicted by vgic_add_lpi() we will simply not find it via xa_for_each().

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