Re: [PATCH v2] KVM: x86: Destroy the PIC and IOAPIC before destroying vCPUs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free on device error path in kvm_create_pit().
--

commit 738d33431d0b6932537e95c21eed6b24c1201219
Author: Weiming Shi <bestswngs@gmail.com>

KVM: x86: Destroy the PIC and IOAPIC before destroying vCPUs

This commit moves the destruction of the in-kernel PIC and IOAPIC to
kvm_arch_pre_destroy_vm(), before vCPUs are freed. This ensures that the
eoi_inject work is cancelled while target APICs remain valid. It prevents
a use-after-free where a delayed work item could dereference a freed vCPU's
APIC map.

Link: https://lore.kernel.org/all/88ba60ad32ba851426a3f6590b0e402210991b4a.e33b58ce.0008.4e1c.aa62.c1024b242cbf@bytedance.com/

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index afcac1042947a..38ac0e0a3f769 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13428,10 +13428,14 @@ void kvm_arch_pre_destroy_vm(struct kvm *kvm)
>  	 * Stop all background workers and kthreads before destroying vCPUs, as
>  	 * iterating over vCPUs in a different task while vCPUs are being freed
>  	 * is unsafe, i.e. will lead to use-after-free.  The PIT also needs to
> -	 * be stopped before IRQ routing is freed.
> +	 * be stopped before IRQ routing is freed.  The PIC and IOAPIC need to
> +	 * be destroyed here too, for the same reason, and because they must
> +	 * be destroyed before the KVM buses are torn down.
>  	 */
>  #ifdef CONFIG_KVM_IOAPIC
>  	kvm_free_pit(kvm);
[Severity: High]
This isn't a bug introduced by this patch, but since we are touching the PIT
and IOAPIC destruction paths here, is there a potential use-after-free on the
device error path in kvm_create_pit()?

If host userspace creates a VM, initializes the IRQCHIP, creates a vCPU,
and starts running it, the vCPU can perform PIO exits, calling
kvm_io_bus_read() and acquiring srcu_read_lock(&kvm->srcu).

If userspace concurrently calls KVM_CREATE_PIT2 with the
KVM_PIT_SPEAKER_DUMMY flag, the kernel registers pit->dev on the PIO bus.
The vCPU thread could pick this up.

If the subsequent registration of pit->speaker_dev fails, the error path
unregisters pit->dev but frees it without waiting for SRCU readers:

arch/x86/kvm/i8254.c:kvm_create_pit() {
    ...
fail_register_speaker:
	kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev);
fail_register_pit:
	mutex_unlock(&kvm->slots_lock);
	kvm_pit_set_reinject(pit, false);
	kthread_destroy_worker(pit->worker);
fail_kthread:
	kfree(pit);
	return NULL;
}

Could the vCPU thread, while iterating over the old bus, dereference
pit->dev->ops and access the freed memory?

> +	kvm_pic_destroy(kvm);
> +	kvm_ioapic_destroy(kvm);
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706180025.2735341-3-bestswngs@gmail.com?part=1