Re: [PATCH] KVM: arm64: Handle race between interrupt affinity change and LPI disabling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent interrupt injection during `vgic_prune_ap_list()`'s dropped-lock window restores the pending state, leading to a poisoned double `list_del` and subsequent Use-After-Free.
--

commit 4bcf4a77aa55dfeba5b7f8ccf0345e4f29d70e1b
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: Handle race between interrupt affinity change and LPI disabling

This patch addresses a use-after-free race condition when migrating an
interrupt while its LPIs are being disabled. It takes an explicit reference
on the interrupt before dropping locks in vgic_prune_ap_list(). It also
removes the migration of the pending bit state.

Link: https://lore.kernel.org/r/ailsCnyoS82r_QRz@v4bel

> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 1e9fe8764584d..ab267d2c0bf8b 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -204,6 +204,7 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
>  	list_for_each_entry_safe(irq, tmp, &vgic_cpu->ap_list_head, ap_list) {
>  		if (irq_is_lpi(vcpu->kvm, irq->intid)) {
>  			raw_spin_lock(&irq->irq_lock);
> +			irq->pending_latch = false;
>  			list_del(&irq->ap_list);
>  			irq->vcpu = NULL;
>  			raw_spin_unlock(&irq->irq_lock);

[ ... ]

> @@ -795,7 +796,11 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
>  			continue;
>  		}
>  
> -		/* This interrupt looks like it has to be migrated. */
> +		/*
> +		 * This interrupt looks like it has to be migrated,
> +		 * make sure it is kept alive while locks are dropped.
> +		 */
> +		vgic_get_irq_ref(irq);

[Severity: Critical]
This isn't a bug introduced by this patch, but could the lock-dropping window
here lead to a double list_del() and a use-after-free under certain race
conditions?

When vgic_prune_ap_list() drops the irq and ap_list locks shortly after this
reference is taken, another thread could concurrently execute
vgic_flush_pending_lpis() above and remove the irq from the list.

If a third thread then concurrently injects the same LPI before the locks are
reacquired, the irq_lock is taken and pending_latch is set to true again.

When vgic_prune_ap_list() reacquires the locks further down in the function,
the target_vcpu oracle check will pass because the interrupt is pending again:

arch/arm64/kvm/vgic/vgic.c:vgic_prune_ap_list() {
    ...
    if (target_vcpu == vgic_target_oracle(irq)) {
        struct vgic_cpu *new_cpu = &target_vcpu->arch.vgic_cpu;

        list_del(&irq->ap_list);
        irq->vcpu = target_vcpu;
    ...
}

Because there is no check verifying that irq->vcpu is still equal to the
source vcpu, does this unconditionally execute list_del() on a list entry
that has already been removed and poisoned?

Would this also bypass acquiring a new list reference, leaving the LPI on the
new list with a deficient reference count if it avoids a crash?

>  
>  		raw_spin_unlock(&irq->irq_lock);
>  		raw_spin_unlock(&vgic_cpu->ap_list_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615181625.3029352-1-maz@kernel.org?part=1