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

["Carlos López" <clopez@suse.de>]

Hi,

I was about to reply to Sashiko, I'd rather talk to a human :)

On 7/3/26 10:44 AM, Oliver Upton wrote:
> 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.

I think we can just use refcount_dec_and_lock_irqsave(), no? Then we
grab the lock only if the refcount drops to 0.

As for the other issue (spurious -ENOMEM on __xa_store()), it's a
preexisting issue, but should be fixed by just passing GFP_NOWAIT to
__xa_store(). I can add another patch in v3 for this.

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


I think this would solve the direct release path, but not the deferred
path. If vgic_add_lpi() does not perform any cleanup, and encounters an
IRQ that was vgic_put_irq_norelease()-ed before
vgic_release_deleted_lpis() grabs the xarray lock then the struct is
overwritten without being released.

> 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);
>  }
>