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

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

[Marc Zyngier]

Hyunwoo Kim reports some really bad races should the following
situation occur:

- LPI-I is pending in vcpu-B's AP list
- vcpu-A writes to vcpu-B's RD to disable its LPIs
- vcpu-C moves I from B to C

If the last two race nicely enough, vgic_prune_ap_list() can drop
the irq and AP list locks, reacquire them, and in the interval
the irq has been freed. UAF follows.

The fix is two-fold:

- Before dropping the irq and ap_list locks, take a reference on
  the irq

- Do not try to handle migration of the pending bit: there is no
  expectation that this state is retained, as per the architecture

With that, we're sure that the interrupt is still around, and we
safely remove it from the AP list as it has no target at this
stage (unless another interrupt fires, but that's another story).

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Tested-by: Hyunwoo Kim <imv4bel@gmail.com>
Link: https://lore.kernel.org/r/ailsCnyoS82r_QRz@v4bel
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 5a4768d8cd4f3..70a161383e5a6 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -203,6 +203,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);
@@ -792,7 +793,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);
 
 		raw_spin_unlock(&irq->irq_lock);
 		raw_spin_unlock(&vgic_cpu->ap_list_lock);
@@ -836,6 +841,8 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
 		raw_spin_unlock(&vcpuA->arch.vgic_cpu.ap_list_lock);
 
+		deleted_lpis |= vgic_put_irq_norelease(vcpu->kvm, irq);
+
 		if (target_vcpu_needs_kick) {
 			kvm_make_request(KVM_REQ_IRQ_PENDING, target_vcpu);
 			kvm_vcpu_kick(target_vcpu);
-- 
2.47.3

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

[sashiko-bot]

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