Re: [PATCH 2/2] KVM: arm64: vgic-v3: Don't drop a pending LPI mapped with LPIs disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: arm64: vgic-v3: unconsumed pending table bit when LPIs are enabled
- [High] KVM: arm64: vgic-v3: TOCTOU race on vgic_lpis_enabled() drops pending LPI
--

commit 1318a5e945ab06141897d5ffe8f4348f6a0ecf31
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: vgic-v3: Don't drop a pending LPI mapped with LPIs disabled

This commit updates vgic_v3_lpi_sync_pending_status() so that it only clears
the pending bit in the guest redistributor's pending table when LPIs are
enabled on the target vCPU. This ensures that when LPIs are subsequently
enabled and its_sync_lpi_pending_table() reloads pending state from guest
memory, a pending LPI mapped while LPIs were disabled does not lose its
pending status.

> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 346bacb3198f2..65b77fb35be37 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -552,7 +552,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
>  	irq->pending_latch = status;
>  	vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
>  
> -	if (status) {
> +	/* Enabling LPIs resets the latch from the table: leave the bit for it */
> +	if (status && vgic_lpis_enabled(vcpu)) {

[Severity: High]
When vgic_lpis_enabled(vcpu) is false, vgic_v3_lpi_sync_pending_status() leaves
the pending bit set in the guest's pending table for its_sync_lpi_pending_table()
to consume later.

However, when the guest enables LPIs on the redistributor via
vgic_mmio_write_v3r_ctlr() -> vgic_enable_lpis() -> its_sync_lpi_pending_table(),
its_sync_lpi_pending_table() reads the pending bit into irq->pending_latch
without clearing the bit in guest memory:

arch/arm64/kvm/vgic/vgic-its.c:its_sync_lpi_pending_table() {
	...
	raw_spin_lock_irqsave(&irq->irq_lock, flags);
	if (irq->target_vcpu == vcpu)
		irq->pending_latch = pendmask & (1U << bit_nr);
	vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
	vgic_put_irq(vcpu->kvm, irq);
	...
}

Could leaving that bit set in guest RAM cause spurious LPI delivery in the
following cases?

1. After the guest acknowledges and deactivates the LPI, only irq->pending_latch
   is cleared in software while the bit in guest RAM remains 1. If the guest
   later discards and remaps the LPI while LPIs are enabled, vgic_add_lpi() ->
   vgic_v3_lpi_sync_pending_status() will read the stale bit from guest RAM and
   inject a spurious interrupt (or if the guest disables and re-enables LPIs,
   its_sync_lpi_pending_table() will re-inject the already-consumed interrupt).

2. If the guest issues an ITS CLEAR command (vgic_its_cmd_handle_clear())
   before enabling LPIs, CLEAR only clears irq->pending_latch, and enabling
   LPIs via its_sync_lpi_pending_table() will resurrect the cleared interrupt
   from the bit left in guest RAM.

[Severity: High]
Can a race between vgic_v3_lpi_sync_pending_status() on one vCPU and
vgic_mmio_write_v3r_ctlr() enabling LPIs on another vCPU cause the pending LPI
to still be dropped?

In vgic_v3_lpi_sync_pending_status(), vgic_queue_irq_unlock() calls
vgic_target_oracle(irq) while holding irq->irq_lock:

arch/arm64/kvm/vgic/vgic.c:vgic_target_oracle() {
	...
	if (unlikely(vcpu && irq_is_lpi(vcpu->kvm, irq->intid) && !vgic_lpis_enabled(vcpu)))
		return NULL;
	...
}

If vgic_lpis_enabled(vcpu) is false at that point, vgic_target_oracle() returns
NULL and vgic_queue_irq_unlock() unlocks irq->irq_lock without queueing the LPI
onto the target vCPU's ap_list.

If another vCPU concurrently writes GICR_CTLR_ENABLE_LPIS in
vgic_mmio_write_v3r_ctlr() right after vgic_queue_irq_unlock() checks
vgic_lpis_enabled(vcpu):

CPU 0 (vgic_v3_lpi_sync_pending_status)     CPU 1 (vgic_mmio_write_v3r_ctlr)
---------------------------------------     --------------------------------
vgic_queue_irq_unlock()
  vgic_target_oracle()
    !vgic_lpis_enabled(vcpu) -> returns NULL
  unlocks irq->irq_lock (not queued)
                                            atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
                                                0, GICR_CTLR_ENABLE_LPIS);
if (status && vgic_lpis_enabled(vcpu))
  /* now true! */
  val &= ~(1 << bit_nr);
  vgic_write_guest_lock(kvm, ptr, &val, 1);
                                            vgic_enable_lpis(vcpu)
                                              its_sync_lpi_pending_table(vcpu)
                                                /* reads 0 from guest table */
                                                irq->pending_latch = 0;

Wouldn't CPU 0 observe vgic_lpis_enabled(vcpu) as true on the second check and
clear the bit in the guest pending table before its_sync_lpi_pending_table()
reads it, causing its_sync_lpi_pending_table() to overwrite irq->pending_latch
with 0 and lose the pending interrupt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026092802.2636798-1-fuad.tabba@linux.dev?part=2