Re: [PATCH v4 1/3] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: loongarch: UAF in irqchip teardown due to missing irq_srcu synchronization
- [High] kvm: loongarch: NULL pointer dereference in IRQ injection paths
--

commit 2f10d82f92df54fcbd750081f5d2c1a3283f66b9
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

The destroy callbacks of the four irqchip devices free the device
structure without clearing kvm->arch.{ipi,eiointc,pch_pic,dmsintc},
leaving a dangling pointer. This clears the pointer when destroying.

> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> --- a/arch/loongarch/kvm/intc/dmsintc.c
> +++ b/arch/loongarch/kvm/intc/dmsintc.c
> @@ -146,6 +146,7 @@ static void kvm_dmsintc_destroy(struct kvm_device *dev)
>  		return;
>  
>  	kfree(dev->kvm->arch.dmsintc);
> +	dev->kvm->arch.dmsintc = NULL;

[Severity: High]
Does this sequence free the structure before setting the pointer to NULL?
This could leave a globally visible dangling pointer during the race window.

If KVM_CREATE_DEVICE fails during anon_inode_getfd() (e.g., due to a process
hitting RLIMIT_NOFILE), this destroy callback executes immediately. If a
guest concurrently issues KVM_SIGNAL_MSI or KVM_IRQ_LINE ioctls, can they
race with this teardown?

Without calling synchronize_srcu(&kvm->irq_srcu) before kfree(), concurrent
IRQ injections holding the SRCU read lock might access the freed memory,
leading to a use-after-free.

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> --- a/arch/loongarch/kvm/intc/eiointc.c
> +++ b/arch/loongarch/kvm/intc/eiointc.c
> @@ -675,6 +675,7 @@ static void kvm_eiointc_destroy(struct kvm_device *dev)
>  
>  	kvm = dev->kvm;
>  	eiointc = kvm->arch.eiointc;
> +	kvm->arch.eiointc = NULL;
>  	mutex_lock(&kvm->slots_lock);
>  	kvm_io_bus_unregister_dev(kvm, KVM_IOCSR_BUS, &eiointc->device);
>  	kvm_io_bus_unregister_dev(kvm, KVM_IOCSR_BUS, &eiointc->device_vext);

[Severity: High]
Can this modification introduce a NULL pointer dereference in the IRQ
injection paths?

When kvm->arch.eiointc is cleared here without SRCU synchronization,
concurrent IRQ routing callbacks might see a NULL pointer. Functions like
eiointc_set_irq() or pch_msi_set_irq() currently dereference
kvm->arch.eiointc unconditionally:

arch/loongarch/kvm/intc/eiointc.c:eiointc_set_irq() {
    ...
    spin_lock_irqsave(&s->lock, flags);
    ...
}

Additionally, since eiointc is eventually freed in this function without
waiting for synchronize_srcu(&kvm->irq_srcu), could concurrent injections
also hit a use-after-free if they managed to read the pointer before it
was set to NULL?

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -483,6 +483,7 @@ static void kvm_pch_pic_destroy(struct kvm_device *dev)
>  
>  	kvm = dev->kvm;
>  	s = kvm->arch.pch_pic;
> +	kvm->arch.pch_pic = NULL;
>  	/* unregister pch pic device and free it's memory */
>  	mutex_lock(&kvm->slots_lock);
>  	kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &s->device);

[Severity: High]
Similar to eiointc, does clearing kvm->arch.pch_pic without SRCU
synchronization expose concurrent IRQ handlers to a NULL pointer dereference
or use-after-free?

If KVM_IRQ_LINE is called concurrently, kvm_set_pic_irq() reads
kvm->arch.pch_pic and passes it to pch_pic_set_irq() without validation:

arch/loongarch/kvm/irqfd.c:kvm_set_pic_irq() {
    ...
    pch_pic_set_irq(kvm->arch.pch_pic, e->irqchip.pin, level);
    ...
}

arch/loongarch/kvm/intc/pch_pic.c:pch_pic_set_irq() {
    ...
    spin_lock(&s->lock);
    ...
}

If the caller reads the pointer after it is cleared, it will dereference
NULL. If it reads the pointer before it is cleared, the subsequent kfree(s)
at the end of this destroy callback could cause a use-after-free, since
there is no synchronize_srcu(&kvm->irq_srcu) to protect the read-side.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015234638.1077815-1-cui.tao@linux.dev?part=1