Re: [PATCH v2 2/2] KVM: arm64: vgic-its: Remove the pending state of a discarded LPI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] kvm: arm64: vgic-its: TOCTOU race resurrects discarded LPI due to IPI skip
- [High] kvm: arm64: vgic-its: Race on vcpu->arch.pause breaks mutual exclusion
- [Medium] kvm: arm64: vgic-its: Global VM halt on DISCARD causes Denial of Service
--

commit 4bb7971d12d2fa6f1083c9f2ddc383b8d63483fb
Author: Fuad Tabba <fuad.tabba@linux.dev>
KVM: arm64: vgic-its: Remove the pending state of a discarded LPI

This commit ensures the pending state of a queued LPI is cleared when handling
an ITS DISCARD command. It halts the guest before clearing the pending state if
the LPI is queued on a running vCPU.

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -829,12 +829,26 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
>  		struct its_device *device = find_its_device(its, device_id);
>  		int ite_esz = vgic_its_get_abi(its)->ite_esz;
>  		gpa_t gpa = device->itt_addr + ite->event_id * ite_esz;
> +		struct vgic_irq *irq = ite->irq;
> +		bool queued;
> +
> +		vgic_its_invalidate_cache(its);
> +
>  		/*
> -		 * Though the spec talks about removing the pending state, we
> -		 * don't bother here since we clear the ITTE anyway and the
> -		 * pending state is a property of the ITTE struct.
> +		 * A queued LPI can be pending in a running vCPU's list register,
> +		 * which the exit path folds back: halt the guest before clearing.
>  		 */
> -		vgic_its_invalidate_cache(its);
> +		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> +			queued = irq->vcpu;
> +
> +		if (queued)
> +			kvm_arm_halt_guest(kvm);

[Severity: Medium]
Does this introduce a denial of service if a guest spams DISCARD commands?

Because kvm_arm_halt_guest() forces all vCPUs in the VM to exit, a guest
rapidly issuing DISCARD commands for a single vCPU's LPI could repeatedly
lock up the entire VM and cause host starvation. Could we restrict the
synchronization strictly to the target irq->vcpu where the LPI is queued?

> +
> +		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> +			irq->pending_latch = false;

[Severity: High]
Is there a window here where an exiting vCPU might overwrite the cleared
latch and resurrect the LPI?

If the target vCPU is just exiting when kvm_arm_halt_guest() broadcasts
KVM_REQ_SLEEP, the vCPU sets vcpu->mode = OUTSIDE_GUEST_MODE before folding
its hardware state in kvm_arch_vcpu_ioctl_run().

Because kvm_request_needs_ipi() checks:

virt/kvm/kvm_main.c:kvm_request_needs_ipi() {
    if (req & KVM_REQUEST_WAIT)
        return mode != OUTSIDE_GUEST_MODE;
}

The IPI is skipped and kvm_arm_halt_guest() returns immediately without
waiting for the vCPU to sync. The DISCARD thread then clears
irq->pending_latch = false.

Concurrently, the exiting vCPU executes kvm_vgic_sync_hwstate(), reads the
still-pending state from the hardware LR, and unconditionally overwrites the
cleared latch back to true:

arch/arm64/kvm/vgic/vgic-v3.c:vgic_v3_fold_lr() {
    if (irq->config == VGIC_CONFIG_EDGE && (val & ICH_LR_PENDING_BIT))
        irq->pending_latch = true;
}

Could we coordinate with the vCPU's state folding sequence to ensure the
discard isn't lost?

> +
> +		if (queued)
> +			kvm_arm_resume_guest(kvm);

[Severity: High]
Can concurrent DISCARD commands on different ITS instances break the mutual
exclusion of vcpu->arch.pause?

vgic_its_cmd_handle_discard() executes holding the per-ITS its->its_lock.
If a VM has multiple ITS devices, two DISCARD commands could run concurrently
and both call kvm_arm_halt_guest(), which sets vcpu->arch.pause to true
across all vCPUs.

The first thread to finish will call kvm_arm_resume_guest(), which does:

arch/arm64/kvm/arm.c:kvm_arm_resume_guest() {
    kvm_for_each_vcpu(i, vcpu, kvm) {
        vcpu->arch.pause = false;
        __kvm_vcpu_wake_up(vcpu);
    }
}

This unconditionally clears the pause flag and wakes up all vCPUs while the
second thread is still modifying VGIC state assuming the guest is fully
halted.

>  
>  		its_free_ite(kvm, ite);
>

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