mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v2 0/1] KVM: arm64: vgic: fix UAF/crash on remote LPI disable
@ 2026-09-18  4:02 zjamg
  2026-09-18  4:02 ` [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable zjamg
  2026-09-20  1:49 ` [PATCH v3 0/1] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay Yuchao Zhang
  0 siblings, 2 replies; 8+ messages in thread
From: zjamg @ 2026-09-18  4:02 UTC (permalink / raw)
  To: Marc Zyngier, Oliver Upton
  Cc: James Morse, Suzuki K Poulose, Zenghui Yu, Catalin Marinas,
	Will Deacon, kvmarm, linux-arm-kernel, linux-kernel,
	Yuchao Zhang

From: Yuchao Zhang <ndaugoing@gmail.com>

Hi Marc, Oliver, and KVM/arm64 maintainers,

By code inspection of commit 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount
deactivations from AP-list tail"), a race condition exists when a remote
vCPU disables LPIs while the target vCPU has an in-flight LPI in a List
Register (LR).

Specifically:
- vgic_flush_pending_lpis() unconditionally unlinks all LPIs from ap_list
  without checking whether the interrupt is in an LR (irq->on_lr).
- If the LPI in the LR happened to be the last one populated, the per-CPU
  pointer *host_data_ptr(last_lr_irq) on the target vCPU is left dangling.
- When the target vCPU exits guest mode, vgic_v3_fold_lr_state() starts
  traversing ap_list via list_for_each_entry_continue() from this unlinked,
  poisoned (or freed) last_lr_irq, leading to UAF or an immediate panic
  when locking irq->irq_lock.

Solution & Scope:
This patch prevents unlinking LPIs that are currently on an LR in
vgic_flush_pending_lpis(), ensures *host_data_ptr(last_lr_irq) is cleared
after folding, skips the ap_list walk when eoicount is zero, and prevents
the fold from resurrecting the pending state of an edge LPI once the
redistributor has LPIs disabled.

Note: this closes the primary race (the last_lr_irq node itself is no
longer unlinkable while in-flight), but the fold traversal can still
race with a remote flush unlinking a subsequent non-LR node in the
ap_list tail. Fully closing that window needs the fold side to take
references before dropping locks (in the spirit of the prune-side fix
in commit 7258770e5814 ("KVM: arm64: vgic: Handle race between
interrupt affinity change and LPI disabling")) and is left as a
follow-up.

Changes in v2:
- Added the fold-side guard: vgic_v3_fold_lr() no longer preserves the
  pending bit of an edge LPI folded while the redistributor has LPIs
  disabled. Without this, a flushed in-flight LPI is resurrected from
  the LR pending bit and re-injected while GICR_CTLR.EnableLPIs is 0
  (spurious LPI delivery to the guest), since the injection path has no
  lpis_enabled gate. Thanks to the Sashiko AI review for pointing this
  out.

Yuchao Zhang (1):
  KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable

 arch/arm64/kvm/vgic/vgic-v2.c |  3 +++
 arch/arm64/kvm/vgic/vgic-v3.c | 14 ++++++++++++--
 arch/arm64/kvm/vgic/vgic.c    | 10 +++++++---
 3 files changed, 22 insertions(+), 5 deletions(-)

--
2.53.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable
  2026-09-18  4:02 [PATCH v2 0/1] KVM: arm64: vgic: fix UAF/crash on remote LPI disable zjamg
@ 2026-09-18  4:02 ` zjamg
  2026-09-18  7:22   ` Fuad Tabba
  2026-09-20  1:49 ` [PATCH v3 0/1] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay Yuchao Zhang
  1 sibling, 1 reply; 8+ messages in thread
From: zjamg @ 2026-09-18  4:02 UTC (permalink / raw)
  To: Marc Zyngier, Oliver Upton
  Cc: James Morse, Suzuki K Poulose, Zenghui Yu, Catalin Marinas,
	Will Deacon, kvmarm, linux-arm-kernel, linux-kernel,
	Yuchao Zhang, stable

From: Yuchao Zhang <ndaugoing@gmail.com>

By code inspection, a race condition and potential Use-After-Free/crash
exists between remote LPI disabling and local LR folding when EOImode==0.

Commit 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount deactivations from
AP-list tail") introduced tracking the last interrupt placed in a List
Register in per-CPU host data (*host_data_ptr(last_lr_irq)) and traverses
the remaining ap_list via list_for_each_entry_continue() in
vgic_v3_fold_lr_state().

However, an interrupt loaded into an LR can be an LPI. While vCPU-B is
running the guest, another vCPU-A can write to vCPU-B's redistributor
GICR_CTLR to clear EnableLPIs, which dispatches vgic_flush_pending_lpis().

vgic_flush_pending_lpis() unconditionally removes all LPIs from ap_list
without checking whether the interrupt is currently in an LR
(irq->on_lr):
- It calls list_del(&irq->ap_list), setting ap_list.next to LIST_POISON1,
  and drops the AP-list reference.
- If the LPI is unmapped or its translation cache was invalidated, the
  vgic_irq refcount drops to zero and the object is freed via RCU.
- Meanwhile, vCPU-B's per-CPU *host_data_ptr(last_lr_irq) cannot be
  cleared by remote vCPUs and is left dangling.

When vCPU-B subsequently exits the guest:
1. vgic_v3_fold_lr_state() resumes using the unlinked last_lr_irq.
2. list_for_each_entry_continue() unconditionally evaluates
   list_next_entry(irq, ap_list) during loop initialization, accessing
   LIST_POISON1 (or freed memory).
3. If eoicount > 0, it attempts guard(raw_spinlock)(&irq->irq_lock) on
   the poisoned address, leading to an immediate host kernel panic.

Fix this by:
1. In vgic_flush_pending_lpis(), do not remove LPIs that are currently
   in-flight in an LR (irq->on_lr == true). They will be naturally pruned
   by the owning vCPU's vgic_prune_ap_list() after LR folding.
2. In vgic_fold_state(), clear *host_data_ptr(last_lr_irq) after folding
   so that no stale pointer survives past guest execution.
3. In vgic_v3_fold_lr_state() and vgic_v2_fold_lr_state(), bail out
   immediately if eoicount is zero, avoiding unnecessary list_next_entry()
   evaluation.
4. In vgic_v3_fold_lr(), do not resurrect the pending state of an edge
   LPI folded while the redistributor has LPIs disabled.
   vgic_flush_pending_lpis() cannot remotely clear the LRs of a running
   vCPU, so such an LPI would otherwise be requeued and re-injected
   while GICR_CTLR.EnableLPIs is 0, violating the flush semantics (and
   the architecture, which gives no expectation of the pending state
   being retained across a disable).

Note: this closes the primary race (the last_lr_irq node itself is no
longer unlinkable while in-flight), but the fold traversal can still
race with a remote flush unlinking a subsequent non-LR node in the
ap_list tail. Fully closing that window needs the fold side to take
references before dropping locks (in the spirit of the prune-side fix
in commit 7258770e5814 ("KVM: arm64: vgic: Handle race between
interrupt affinity change and LPI disabling")) and is left as a
follow-up.

Fixes: 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount deactivations from AP-list tail")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
 arch/arm64/kvm/vgic/vgic-v2.c |  3 +++
 arch/arm64/kvm/vgic/vgic-v3.c | 14 ++++++++++++--
 arch/arm64/kvm/vgic/vgic.c    | 10 +++++++---
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 7182f63fc938..7b6cd05ce32d 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -122,6 +122,9 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 	for (int lr = 0; lr < vgic_cpu->vgic_v2.used_lrs; lr++)
 		vgic_v2_fold_lr(vcpu, cpuif->vgic_lr[lr]);
 
+	if (!eoicount)
+		return;
+
 	/* See the GICv3 equivalent for the EOIcount handling rationale */
 	list_for_each_entry_continue(irq, &vgic_cpu->ap_list_head, ap_list) {
 		u32 lr;
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 726e20a1da6e..dd1314f4543d 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -96,9 +96,16 @@ static void vgic_v3_fold_lr(struct kvm_vcpu *vcpu, u64 val)
 		deactivated = irq->active && !(val & ICH_LR_ACTIVE_BIT);
 		irq->active = !!(val & ICH_LR_ACTIVE_BIT);
 
-		/* Edge is the only case where we preserve the pending bit */
+		/*
+		 * Edge is the only case where we preserve the pending bit.
+		 * Do not resurrect the pending state of LPIs once the
+		 * redistributor has them disabled: vgic_flush_pending_lpis()
+		 * has discarded them, and the LRs of a running vCPU cannot
+		 * be remotely cleared.
+		 */
 		if (irq->config == VGIC_CONFIG_EDGE &&
-		    (val & ICH_LR_PENDING_BIT))
+		    (val & ICH_LR_PENDING_BIT) &&
+		    (irq->intid < VGIC_MIN_LPI || vgic_lpis_enabled(vcpu)))
 			irq->pending_latch = true;
 
 		/*
@@ -155,6 +162,9 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 	for (int lr = 0; lr < cpuif->used_lrs; lr++)
 		vgic_v3_fold_lr(vcpu, cpuif->vgic_lr[lr]);
 
+	if (!eoicount)
+		return;
+
 	/*
 	 * EOIMode=0: use EOIcount to emulate deactivation. We are
 	 * guaranteed to deactivate in reverse order of the activation, so
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index b25303d9919f..0d7d75c3ac93 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -205,10 +205,12 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
 		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;
+			if (!irq->on_lr) {
+				list_del(&irq->ap_list);
+				irq->vcpu = NULL;
+				deleted |= vgic_put_irq_norelease(vcpu->kvm, irq);
+			}
 			raw_spin_unlock(&irq->irq_lock);
-			deleted |= vgic_put_irq_norelease(vcpu->kvm, irq);
 		}
 	}
 
@@ -873,6 +875,8 @@ static void vgic_fold_state(struct kvm_vcpu *vcpu)
 		vgic_v2_fold_lr_state(vcpu);
 	else
 		vgic_v3_fold_lr_state(vcpu);
+
+	*host_data_ptr(last_lr_irq) = NULL;
 }
 
 /* Requires the irq_lock to be held. */
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable
  2026-09-18  4:02 ` [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable zjamg
@ 2026-09-18  7:22   ` Fuad Tabba
  2026-09-18 11:50     ` Yuchao Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Fuad Tabba @ 2026-09-18  7:22 UTC (permalink / raw)
  To: zjamg
  Cc: Marc Zyngier, Oliver Upton, James Morse, Suzuki K Poulose,
	Zenghui Yu, Catalin Marinas, Will Deacon, kvmarm,
	linux-arm-kernel, linux-kernel, stable

Hi Yuchao,

On Fri, 18 Sep 2026 12:02:14 +0800, Yuchao Zhang <ndaugoing@gmail.com> wrote:

[...]

> Note: this closes the primary race (the last_lr_irq node itself is no
> longer unlinkable while in-flight), but the fold traversal can still
> race with a remote flush unlinking a subsequent non-LR node in the
> ap_list tail. Fully closing that window needs the fold side to take
> references before dropping locks (in the spirit of the prune-side fix
> in commit 7258770e5814 ("KVM: arm64: vgic: Handle race between
> interrupt affinity change and LPI disabling")) and is left as a
> follow-up.

This is the same race Hyunwoo reported back in June [1]. You might
want to have a look at that thread first: Oliver and Marc's view there
was that the fix is to take the ap_list_lock in
vgic_v3_fold_lr_state() [2][3], and Hyunwoo posted a draft of that
[4].

Cheers,
/fuad

[1] https://lore.kernel.org/r/aiHrGM1f8czcUby4@v4bel
[2] https://lore.kernel.org/r/aiJi5a3JJ-TbWL-s@kernel.org
[3] https://lore.kernel.org/r/87a4t99z9n.wl-maz@kernel.org
[4] https://lore.kernel.org/r/aiXvwGD1hS6vwLEd@v4bel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable
  2026-09-18  7:22   ` Fuad Tabba
@ 2026-09-18 11:50     ` Yuchao Zhang
  2026-09-18 11:58       ` Fuad Tabba
  0 siblings, 1 reply; 8+ messages in thread
From: Yuchao Zhang @ 2026-09-18 11:50 UTC (permalink / raw)
  To: Fuad Tabba
  Cc: Marc Zyngier, Oliver Upton, James Morse, Suzuki K Poulose,
	Zenghui Yu, Catalin Marinas, Will Deacon, kvmarm,
	linux-arm-kernel, linux-kernel, stable

Hi Fuad,

Thanks a lot for pointing me to that thread! I was not aware of
Hyunwoo's earlier report and the discussion with Oliver and Marc.

I'll read through the thread and their rationale on the ap_list_lock
approach. I'm happy to defer to Hyunwoo's effort to avoid duplicate
work.

Thanks again for the pointer!

Best regards,
Yuchao

Fuad Tabba <fuad.tabba@linux.dev> 于2026年9月18日周五 15:22写道:
>
> Hi Yuchao,
>
> On Fri, 18 Sep 2026 12:02:14 +0800, Yuchao Zhang <ndaugoing@gmail.com> wrote:
>
> [...]
>
> > Note: this closes the primary race (the last_lr_irq node itself is no
> > longer unlinkable while in-flight), but the fold traversal can still
> > race with a remote flush unlinking a subsequent non-LR node in the
> > ap_list tail. Fully closing that window needs the fold side to take
> > references before dropping locks (in the spirit of the prune-side fix
> > in commit 7258770e5814 ("KVM: arm64: vgic: Handle race between
> > interrupt affinity change and LPI disabling")) and is left as a
> > follow-up.
>
> This is the same race Hyunwoo reported back in June [1]. You might
> want to have a look at that thread first: Oliver and Marc's view there
> was that the fix is to take the ap_list_lock in
> vgic_v3_fold_lr_state() [2][3], and Hyunwoo posted a draft of that
> [4].
>
> Cheers,
> /fuad
>
> [1] https://lore.kernel.org/r/aiHrGM1f8czcUby4@v4bel
> [2] https://lore.kernel.org/r/aiJi5a3JJ-TbWL-s@kernel.org
> [3] https://lore.kernel.org/r/87a4t99z9n.wl-maz@kernel.org
> [4] https://lore.kernel.org/r/aiXvwGD1hS6vwLEd@v4bel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable
  2026-09-18 11:50     ` Yuchao Zhang
@ 2026-09-18 11:58       ` Fuad Tabba
  2026-09-18 19:15         ` Oliver Upton
  0 siblings, 1 reply; 8+ messages in thread
From: Fuad Tabba @ 2026-09-18 11:58 UTC (permalink / raw)
  To: Yuchao Zhang
  Cc: Marc Zyngier, Oliver Upton, James Morse, Suzuki K Poulose,
	Zenghui Yu, Catalin Marinas, Will Deacon, kvmarm,
	linux-arm-kernel, linux-kernel, stable

Hi Yuchao,

On Fri, 18 Sept 2026 at 12:51, Yuchao Zhang <ndaugoing@gmail.com> wrote:
>
> Hi Fuad,
>
> Thanks a lot for pointing me to that thread! I was not aware of
> Hyunwoo's earlier report and the discussion with Oliver and Marc.
>
> I'll read through the thread and their rationale on the ap_list_lock
> approach. I'm happy to defer to Hyunwoo's effort to avoid duplicate
> work.

I'm not sure you should defer to their effort. It doesn't seem like
Hyunwoo has done any work on this for a while. I just wanted to point
you to the existing discussion.

Cheers,
/fuad

> Thanks again for the pointer!
>
> Best regards,
> Yuchao
>
> Fuad Tabba <fuad.tabba@linux.dev> 于2026年9月18日周五 15:22写道:
> >
> > Hi Yuchao,
> >
> > On Fri, 18 Sep 2026 12:02:14 +0800, Yuchao Zhang <ndaugoing@gmail.com> wrote:
> >
> > [...]
> >
> > > Note: this closes the primary race (the last_lr_irq node itself is no
> > > longer unlinkable while in-flight), but the fold traversal can still
> > > race with a remote flush unlinking a subsequent non-LR node in the
> > > ap_list tail. Fully closing that window needs the fold side to take
> > > references before dropping locks (in the spirit of the prune-side fix
> > > in commit 7258770e5814 ("KVM: arm64: vgic: Handle race between
> > > interrupt affinity change and LPI disabling")) and is left as a
> > > follow-up.
> >
> > This is the same race Hyunwoo reported back in June [1]. You might
> > want to have a look at that thread first: Oliver and Marc's view there
> > was that the fix is to take the ap_list_lock in
> > vgic_v3_fold_lr_state() [2][3], and Hyunwoo posted a draft of that
> > [4].
> >
> > Cheers,
> > /fuad
> >
> > [1] https://lore.kernel.org/r/aiHrGM1f8czcUby4@v4bel
> > [2] https://lore.kernel.org/r/aiJi5a3JJ-TbWL-s@kernel.org
> > [3] https://lore.kernel.org/r/87a4t99z9n.wl-maz@kernel.org
> > [4] https://lore.kernel.org/r/aiXvwGD1hS6vwLEd@v4bel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable
  2026-09-18 11:58       ` Fuad Tabba
@ 2026-09-18 19:15         ` Oliver Upton
  0 siblings, 0 replies; 8+ messages in thread
From: Oliver Upton @ 2026-09-18 19:15 UTC (permalink / raw)
  To: Fuad Tabba
  Cc: Yuchao Zhang, Marc Zyngier, Oliver Upton, James Morse,
	Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
	kvmarm, linux-arm-kernel, linux-kernel, stable

On Fri, Sep 18, 2026 at 12:58:17PM +0100, Fuad Tabba wrote:
> Hi Yuchao,
> 
> On Fri, 18 Sept 2026 at 12:51, Yuchao Zhang <ndaugoing@gmail.com> wrote:
> >
> > Hi Fuad,
> >
> > Thanks a lot for pointing me to that thread! I was not aware of
> > Hyunwoo's earlier report and the discussion with Oliver and Marc.
> >
> > I'll read through the thread and their rationale on the ap_list_lock
> > approach. I'm happy to defer to Hyunwoo's effort to avoid duplicate
> > work.
> 
> I'm not sure you should defer to their effort. It doesn't seem like
> Hyunwoo has done any work on this for a while. I just wanted to point
> you to the existing discussion.

Yuchao if you have cycles I would definitely appreciate it if you can
pursue a fix. My view hasn't changed since before: let's make that
traversal of the ap_list is done under the ap_list_lock, as this is not
intended to be walked lock-free.

Taking a step back, the whole cross-vCPU LPI disabling always leaves me
feeling ill... Really when RWP=0 becomes visible from another vCPU we
need to guarantee that the LPIs have been actually retired, meaning we
can't have one sitting in an LR. Even with the locking fix I think we
miss this.

Given how unlikely it is for well-behaved software to disable LPIs
remotely in the first place, I wonder if we should just halt the VM
similar to how we handle accesses to the active state. That's a really
big hammer but we've had a lot of bugs in this department and I'm
somewhat biased towards an obviously correct solution.

We wouldn't need to do this for a vCPU disabling LPIs on its own
redistributor since we've already exited the guest.

I'll think on it a bit more.

Oliver

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 0/1] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay
  2026-09-18  4:02 [PATCH v2 0/1] KVM: arm64: vgic: fix UAF/crash on remote LPI disable zjamg
  2026-09-18  4:02 ` [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable zjamg
@ 2026-09-20  1:49 ` Yuchao Zhang
  2026-09-20  1:49   ` [PATCH v3] " Yuchao Zhang
  1 sibling, 1 reply; 8+ messages in thread
From: Yuchao Zhang @ 2026-09-20  1:49 UTC (permalink / raw)
  To: Marc Zyngier, Oliver Upton
  Cc: Fuad Tabba, James Morse, Suzuki K Poulose, Zenghui Yu,
	Catalin Marinas, Will Deacon, kvmarm, linux-arm-kernel,
	linux-kernel, Yuchao Zhang

Hi Marc, Oliver, Fuad, and KVM/arm64 maintainers,

Following up on the discussion around the remote LPI disable vs LR fold
race [1], this series addresses the issue at its root: the last_lr_irq
cursor introduced in commit 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount
deactivations from AP-list tail").

Problem:
vgic_v3_fold_lr_state() / vgic_v2_fold_lr_state() walk the overflow tail
of the ap_list starting from *host_data_ptr(last_lr_irq) without holding
ap_list_lock. Caching this raw pointer across the entire guest execution
leaves it vulnerable to concurrent modification: when a remote vCPU
disables LPIs via GICR_CTLR, vgic_flush_pending_lpis() unlinks the node
with list_del() and drops its reference, leaving last_lr_irq pointing to
a poisoned or freed object. When the vCPU exits, the walk dereferences
corrupted memory, causing a kernel panic or UAF.

Oliver and Marc suggested taking ap_list_lock in
vgic_v3_fold_lr_state() [2][3]. I tried that approach first, but it
runs into the following lock-order problems:
1. kvm_notify_acked_irq() grabs regular spinlocks and can re-enter
   vgic_queue_irq_unlock() (which takes ap_list_lock), causing deadlock.
2. vgic_put_irq() is a no-op for SPI/PPI, but for LPIs it calls
   refcount_dec_and_lock_irqsave() which acquires dist->lpi_xa.xa_lock
   when dropping the last reference. That lock sits above ap_list_lock
   in the lock ordering, so calling vgic_put_irq() under ap_list_lock
   causes lock inversion.
Addressing these under a global fold lock requires deferring all EOI'ed
SPI notifications to a stack bitmap and deferring LPI releases with
vgic_put_irq_norelease(), penalizing the fast path for all exits even
though folding hardware LRs does not touch ap_list at all. It also still
requires pinning and clearing the per-CPU last_lr_irq pointer.

Changes since v2:
- Replaced the skip-unlink approach of v2 with dropping the last_lr_irq
  cursor entirely and serializing only the overflow EOI replay under
  ap_list_lock (per Oliver and Marc's suggestion [2][3]).

The cleaner approach in this patch:
1. Drop the fragile last_lr_irq per-CPU cursor entirely.
2. The common fast path (folding hardware LRs) runs natively without
   ap_list_lock. We record the INTIDs of the used LRs in a small stack
   array (VGIC_V3_MAX_LRS / VGIC_V2_MAX_LRS entries).
3. If eoicount == 0 (the vast majority of guest exits), clear
   cpuif->used_lrs = 0 and return immediately without taking ap_list_lock.
4. If unlikely(eoicount > 0), acquire ap_list_lock only to scan the ap_list
   and pin (via vgic_get_irq_ref) up to eoicount active interrupts that
   were not in hardware LRs. The scan is a linear walk over at most 16/64
   LR INTIDs per candidate, on the rare eoicount > 0 path - bounded and
   acceptable. lr_intids[] only records INTIDs from the used_lrs range;
   the extraction mask mirrors vgic_fold_lr() exactly
   (ICH_LR_VIRTUAL_ID_MASK for GICv3, GICH_LR_VIRTUALID for GICv2),
   so no stale or invalid slot can produce a false match.
5. Drop ap_list_lock immediately, and then replay their deactivations
   outside the lock, naturally eliminating both eventfd re-entrancy and
   lpi_xa lock inversions without changing any function signatures.
   vgic_fold_lr() has no error path, so cpuif->used_lrs = 0 is always
   reached after a complete fold, with no risk of partial cleanup.

Note on EOIcount hardware limits:
ICH_HCR_EL2.EOIcount (GICv3) and GICH_HCR.EOICount (GICv2) are both
5-bit fields, giving a maximum value of 31. The targets[32] stack array
and min_t(u32, eoicount, ARRAY_SIZE(targets)) bound together ensure no
overflow even if hardware writes an unexpected value.

Note on EOIcount source:
For GICv3, eoicount is read from cpuif->vgic_hcr, which is populated by
__vgic_v3_save_state right before ICH_HCR_EL2 is cleared in hardware.
For GICv2, vgic_v2_save_state reads GICH_HCR via MMIO into the same
cpuif->vgic_hcr field (when LRENPIE is set) before writing 0 to GICH_HCR.
In both cases the software copy is the only valid source; reading the
hardware register after save would return 0.

Note on scope:
This series fixes the use-after-free in the ap_list traversal caused by
last_lr_irq. It does not address the separate concern raised by Oliver
in [2] about a pending LPI still sitting in an LR when RWP=0 becomes
visible to another vCPU; that may require a stronger approach (e.g.
halting the VM) and I am happy to follow up separately.

[1] https://lore.kernel.org/r/aiHrGM1f8czcUby4@v4bel
[2] https://lore.kernel.org/r/aiJi5a3JJ-TbWL-s@kernel.org
[3] https://lore.kernel.org/r/87a4t99z9n.wl-maz@kernel.org

Yuchao Zhang (1):
  KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay

 arch/arm64/include/asm/kvm_host.h |  3 --
 arch/arm64/kvm/vgic/vgic-v2.c     | 64 +++++++++++++++++++++++--------
 arch/arm64/kvm/vgic/vgic-v3.c     | 74 ++++++++++++++++++++++++++-----
 arch/arm64/kvm/vgic/vgic.c        |  9 +---
 arch/arm64/kvm/vgic/vgic.h        | 14 ++++++++
 5 files changed, 114 insertions(+), 50 deletions(-)

-- 
2.53.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay
  2026-09-20  1:49 ` [PATCH v3 0/1] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay Yuchao Zhang
@ 2026-09-20  1:49   ` Yuchao Zhang
  0 siblings, 0 replies; 8+ messages in thread
From: Yuchao Zhang @ 2026-09-20  1:49 UTC (permalink / raw)
  To: Marc Zyngier, Oliver Upton
  Cc: Fuad Tabba, James Morse, Suzuki K Poulose, Zenghui Yu,
	Catalin Marinas, Will Deacon, kvmarm, linux-arm-kernel,
	linux-kernel, Yuchao Zhang, stable

Commit 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount deactivations from
AP-list tail") introduced tracking the last interrupt placed in a List
Register in per-CPU host data (*host_data_ptr(last_lr_irq)) to resume the
EOIcount-based deactivation walk in the overflow tail of the ap_list.

However, caching this raw pointer in per-CPU host data spans the entire
guest execution without holding ap_list_lock or a reference count.
A concurrent vCPU can disable LPIs by writing to GICR_CTLR.EnableLPIs,
triggering vgic_flush_pending_lpis() which removes LPIs with list_del()
and drops their reference.  When the victim vCPU exits, the lock-free
list_for_each_entry_continue() starting from last_lr_irq dereferences a
poisoned next pointer or freed object, causing a host panic or UAF.

Rather than taking ap_list_lock across the entire fold path (which forces
deferring kvm_notify_acked_irq() to avoid eventfd deadlocks and deferring
vgic_put_irq() to avoid lock inversion with lpi_xa), solve this by
dropping the fragile last_lr_irq cursor entirely:

1. In vgic_flush_lr_state(), do not track last_lr_irq.
2. In vgic_v3_fold_lr_state() / vgic_v2_fold_lr_state(), fold the hardware
   LRs natively without ap_list_lock.  Record the INTIDs of the used LRs in
   a small stack array (at most 16/64 entries).
3. If eoicount == 0 (the vast majority of exits), clear cpuif->used_lrs and
   return immediately without taking ap_list_lock on the fast path.
4. If unlikely(eoicount > 0), acquire ap_list_lock only to scan the ap_list
   and pin up to eoicount active interrupts that were not in hardware LRs.
   The scan is a linear walk over at most 16/64 LR INTIDs per candidate, on
   the rare eoicount > 0 path - bounded and acceptable.
   Then drop ap_list_lock before replaying their deactivations, naturally
   avoiding both eventfd re-entrancy and lpi_xa lock inversions.

Fixes: 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount deactivations from AP-list tail")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
 arch/arm64/include/asm/kvm_host.h |  3 --
 arch/arm64/kvm/vgic/vgic-v2.c     | 64 +++++++++++++++++++-------
 arch/arm64/kvm/vgic/vgic-v3.c     | 74 +++++++++++++++++++++----------
 arch/arm64/kvm/vgic/vgic.c        |  9 +---
 arch/arm64/kvm/vgic/vgic.h        | 14 ++++++
 5 files changed, 114 insertions(+), 50 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 27fe0cd5b2d7..c4355ae23e98 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -800,9 +800,6 @@ struct kvm_host_data {
 	unsigned int debug_brps;
 	unsigned int debug_wrps;
 
-	/* Last vgic_irq part of the AP list recorded in an LR */
-	struct vgic_irq *last_lr_irq;
-
 	/* PPI state tracking for GICv5-based guests */
 	struct {
 		DECLARE_BITMAP(pendr, VGIC_V5_NR_PRIVATE_IRQS);
diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 7182f63fc938..4a0ad5bd1cf6 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -115,37 +115,71 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_v2_cpu_if *cpuif = &vgic_cpu->vgic_v2;
 	u32 eoicount = FIELD_GET(GICH_HCR_EOICOUNT, cpuif->vgic_hcr);
-	struct vgic_irq *irq = *host_data_ptr(last_lr_irq);
+	struct vgic_irq *targets[32];
+	u32 lr_intids[VGIC_V2_MAX_LRS];
+	int nr_lrs = min_t(int, vgic_cpu->vgic_v2.used_lrs, ARRAY_SIZE(lr_intids));
+	u32 max_targets;
+	int nr_targets = 0;
 
 	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
 
-	for (int lr = 0; lr < vgic_cpu->vgic_v2.used_lrs; lr++)
-		vgic_v2_fold_lr(vcpu, cpuif->vgic_lr[lr]);
+	if (!vgic_cpu->vgic_v2.used_lrs && !eoicount)
+		return;
 
-	/* See the GICv3 equivalent for the EOIcount handling rationale */
-	list_for_each_entry_continue(irq, &vgic_cpu->ap_list_head, ap_list) {
-		u32 lr;
+	for (int lr = 0; lr < nr_lrs; lr++) {
+		u32 val = cpuif->vgic_lr[lr];
+
+		lr_intids[lr] = val & GICH_LR_VIRTUALID;
+		vgic_v2_fold_lr(vcpu, val);
+	}
+
+	cpuif->used_lrs = 0;
+
+	if (likely(!eoicount))
+		return;
+
+	max_targets = min_t(u32, eoicount, ARRAY_SIZE(targets));
+
+	/*
+	 * EOIMode=0: replay deactivations for overflow active interrupts.
+	 * Walk ap_list under ap_list_lock and pin candidate interrupts so we
+	 * can process them outside the lock without risking lock inversion.
+	 */
+	scoped_guard(raw_spinlock, &vgic_cpu->ap_list_lock) {
+		struct vgic_irq *irq;
 
-		if (!eoicount) {
-			break;
-		} else {
-			guard(raw_spinlock)(&irq->irq_lock);
+		list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
+			if (nr_targets == max_targets)
+				break;
 
-			if (!(likely(vgic_target_oracle(irq) == vcpu) &&
-			      irq->active))
+			if (intid_in_lrs(irq->intid, lr_intids, nr_lrs))
 				continue;
 
+			scoped_guard(raw_spinlock, &irq->irq_lock) {
+				if (likely(vgic_target_oracle(irq) == vcpu) &&
+				    irq->active) {
+					vgic_get_irq_ref(irq);
+					targets[nr_targets++] = irq;
+				}
+			}
+		}
+	}
+
+	for (int i = 0; i < nr_targets; i++) {
+		struct vgic_irq *irq = targets[i];
+		u32 lr;
+
+		scoped_guard(raw_spinlock, &irq->irq_lock) {
 			lr = vgic_v2_compute_lr(vcpu, irq) & ~GICH_LR_ACTIVE_BIT;
 		}
 
 		if (lr & GICH_LR_HW)
 			writel_relaxed(FIELD_GET(GICH_LR_PHYSID_CPUID, lr),
 				       kvm_vgic_global_state.gicc_base + GIC_CPU_DEACTIVATE);
+
 		vgic_v2_fold_lr(vcpu, lr);
-		eoicount--;
+		vgic_put_irq(vcpu->kvm, irq);
 	}
-
-	cpuif->used_lrs = 0;
 }
 
 void vgic_v2_deactivate(struct kvm_vcpu *vcpu, u32 val)
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 726e20a1da6e..7e573860f3e1 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -148,37 +148,65 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_v3_cpu_if *cpuif = &vgic_cpu->vgic_v3;
 	u32 eoicount = FIELD_GET(ICH_HCR_EL2_EOIcount, cpuif->vgic_hcr);
-	struct vgic_irq *irq = *host_data_ptr(last_lr_irq);
+	struct vgic_irq *targets[32];
+	u32 lr_intids[VGIC_V3_MAX_LRS];
+	int nr_lrs = min_t(int, cpuif->used_lrs, ARRAY_SIZE(lr_intids));
+	u32 max_targets;
+	int nr_targets = 0;
 
 	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
 
-	for (int lr = 0; lr < cpuif->used_lrs; lr++)
-		vgic_v3_fold_lr(vcpu, cpuif->vgic_lr[lr]);
+	if (!cpuif->used_lrs && !eoicount)
+		return;
+
+	for (int lr = 0; lr < nr_lrs; lr++) {
+		u64 val = cpuif->vgic_lr[lr];
+
+		if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+			lr_intids[lr] = val & ICH_LR_VIRTUAL_ID_MASK;
+		else
+			lr_intids[lr] = val & GICH_LR_VIRTUALID;
+
+		vgic_v3_fold_lr(vcpu, val);
+	}
+
+	cpuif->used_lrs = 0;
+
+	if (likely(!eoicount))
+		return;
+
+	max_targets = min_t(u32, eoicount, ARRAY_SIZE(targets));
 
 	/*
-	 * EOIMode=0: use EOIcount to emulate deactivation. We are
-	 * guaranteed to deactivate in reverse order of the activation, so
-	 * just pick one active interrupt after the other in the tail part
-	 * of the ap_list, past the LRs, and replay the deactivation as if
-	 * the CPU was doing it. We also rely on priority drop to have taken
-	 * place, and the list to be sorted by priority.
+	 * EOIMode=0: replay deactivations for overflow active interrupts.
+	 * Walk ap_list under ap_list_lock and pin candidate interrupts so we
+	 * can process them outside the lock without risking lock inversion.
 	 */
-	list_for_each_entry_continue(irq, &vgic_cpu->ap_list_head, ap_list) {
-		u64 lr;
+	scoped_guard(raw_spinlock, &vgic_cpu->ap_list_lock) {
+		struct vgic_irq *irq;
 
-		/*
-		 * I would have loved to write this using a scoped_guard(),
-		 * but using 'continue' here is a total train wreck.
-		 */
-		if (!eoicount) {
-			break;
-		} else {
-			guard(raw_spinlock)(&irq->irq_lock);
+		list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
+			if (nr_targets == max_targets)
+				break;
 
-			if (!(likely(vgic_target_oracle(irq) == vcpu) &&
-			      irq->active))
+			if (intid_in_lrs(irq->intid, lr_intids, nr_lrs))
 				continue;
 
+			scoped_guard(raw_spinlock, &irq->irq_lock) {
+				if (likely(vgic_target_oracle(irq) == vcpu) &&
+				    irq->active) {
+					vgic_get_irq_ref(irq);
+					targets[nr_targets++] = irq;
+				}
+			}
+		}
+	}
+
+	for (int i = 0; i < nr_targets; i++) {
+		struct vgic_irq *irq = targets[i];
+		u64 lr;
+
+		scoped_guard(raw_spinlock, &irq->irq_lock) {
 			lr = vgic_v3_compute_lr(vcpu, irq) & ~ICH_LR_ACTIVE_BIT;
 		}
 
@@ -186,10 +214,8 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 			vgic_v3_deactivate_phys(FIELD_GET(ICH_LR_PHYS_ID_MASK, lr));
 
 		vgic_v3_fold_lr(vcpu, lr);
-		eoicount--;
+		vgic_put_irq(vcpu->kvm, irq);
 	}
-
-	cpuif->used_lrs = 0;
 }
 
 void vgic_v3_deactivate(struct kvm_vcpu *vcpu, u64 val)
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index b25303d9919f..966948fd3ddd 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -866,9 +866,6 @@ static void vgic_fold_state(struct kvm_vcpu *vcpu)
 		return;
 	}
 
-	if (!*host_data_ptr(last_lr_irq))
-		return;
-
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_fold_lr_state(vcpu);
 	else
@@ -1015,14 +1012,10 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
 	if (irqs_outside_lrs(&als))
 		vgic_sort_ap_list(vcpu);
 
-	*host_data_ptr(last_lr_irq) = NULL;
-
 	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
 		scoped_guard(raw_spinlock,  &irq->irq_lock) {
-			if (likely(vgic_target_oracle(irq) == vcpu)) {
+			if (likely(vgic_target_oracle(irq) == vcpu))
 				vgic_populate_lr(vcpu, irq, count++);
-				*host_data_ptr(last_lr_irq) = irq;
-			}
 		}
 
 		if (count == kvm_vgic_global_state.nr_lr)
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index b71d486ae514..35a07b320be4 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -334,6 +334,20 @@ static inline void vgic_get_irq_ref(struct vgic_irq *irq)
 	WARN_ON_ONCE(!vgic_try_get_irq_ref(irq));
 }
 
+/*
+ * Linear scan over at most VGIC_V3_MAX_LRS / VGIC_V2_MAX_LRS entries.
+ * Only called on the rare eoicount > 0 path, so the O(n) cost is
+ * acceptable and avoids the complexity of a bitmap.
+ */
+static inline bool intid_in_lrs(u32 intid, const u32 *lr_intids, int nr_lrs)
+{
+	for (int i = 0; i < nr_lrs; i++) {
+		if (lr_intids[i] == intid)
+			return true;
+	}
+	return false;
+}
+
 void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
 void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
 void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr);
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-09-20  1:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-18  4:02 [PATCH v2 0/1] KVM: arm64: vgic: fix UAF/crash on remote LPI disable zjamg
2026-09-18  4:02 ` [PATCH v2] KVM: arm64: vgic: Do not remove in-flight LPIs from AP list on disable zjamg
2026-09-18  7:22   ` Fuad Tabba
2026-09-18 11:50     ` Yuchao Zhang
2026-09-18 11:58       ` Fuad Tabba
2026-09-18 19:15         ` Oliver Upton
2026-09-20  1:49 ` [PATCH v3 0/1] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay Yuchao Zhang
2026-09-20  1:49   ` [PATCH v3] " Yuchao Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®