Re: [PATCH v5 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:
- [Critical] kvm: NULL pointer dereference in lockless IRQ injection paths
- [High] kvm: loongarch: UAF window in kvm_dmsintc_destroy due to wrong kfree order
--

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

LoongArch: KVM: Clear device pointer in irqchip destroy callbacks
    
This commit clears the irqchip device pointers when destroying devices to
prevent dangling pointers from being dereferenced if device creation fails
later.

> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> index ff327b4c05f7e..c182185a3d592 100644
> --- 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 create a Use-After-Free window? The memory is freed before the
globally visible pointer is cleared. If KVM_CREATE_DEVICE succeeds but a
subsequent error (e.g. in anon_inode_getfd) triggers ops->destroy(), this
invokes kvm_dmsintc_destroy() while concurrent threads can still access the
pointer.

Should the pointer be cleared before calling kfree(), as is correctly done
for the other irqchip devices in this patch?

>  	kfree(dev);
>  }
>  

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> index 220fc23ffe8cf..dab2d3f1b8f62 100644
> --- 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;

[Severity: Critical]
Does setting this pointer to NULL introduce a NULL pointer dereference in the
concurrent lockless IRQ injection paths?

If a user-space process maps GSI routing to an irqchip and races KVM_IRQ_LINE
or KVM_IRQFD interrupt injection against a failing KVM_CREATE_DEVICE ioctl
(e.g. by hitting the process RLIMIT_NOFILE limit), this teardown path will
assign NULL to the irqchip pointer.

Concurrently, the IRQ injection path locklessly invokes routing callbacks
like kvm_set_pic_irq() which pass the potentially NULL kvm->arch.pch_pic into
pch_pic_set_irq():

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

The pointer is then unconditionally dereferenced:

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

A similar pattern exists for eiointc_set_irq() in
arch/loongarch/kvm/intc/eiointc.c.

Are NULL guard checks needed in the concurrent readers to safely handle this
regression?

>  	/* 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);

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