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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kvm: loongarch: NULL deref and UAF via lockless irqchip destruction
--

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

LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

The commit clears device pointers in the destroy callbacks for the four
LoongArch irqchip devices to prevent dangling pointers. This addresses an
issue where anon_inode_getfd() failures during KVM_CREATE_DEVICE can
leave the VM alive with a freed irqchip device.

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

[Severity: Critical]
Does setting this pointer to NULL after freeing it leave a window open for
a use-after-free?

If anon_inode_getfd() fails during KVM_CREATE_DEVICE, the error path
calls ops->destroy() locklessly. At the same time, concurrent KVM_IRQ_LINE
requests can locklessly invoke kvm_set_irq(), which drops kvm->irq_srcu
before calling the routing callbacks.

If those callbacks read the pointer between the kfree() and this NULL
assignment, won't they dereference freed memory? Does this ordering defeat
any READ_ONCE() fixes intended for concurrent lockless readers?

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> index e7b77705c516a..5d046a1cab4e2 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -480,6 +480,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]
Can assigning NULL here without synchronization lead to a deterministic NULL
pointer dereference and host panic?

If kvm->arch.pch_pic is set to NULL locklessly here, and a concurrent
KVM_IRQ_LINE ioctl calls into kvm_set_pic_irq(), the routing table
execution happens after kvm_set_irq() drops irq_srcu:

virt/kvm/irqchip.c:kvm_set_irq() {
    ...
    idx = srcu_read_lock(&kvm->irq_srcu);
    i = kvm_irq_map_gsi(kvm, irq_set, irq);
    srcu_read_unlock(&kvm->irq_srcu, idx);

    while (i--) {
        int r;
        r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level,
                           line_status);
    ...
}

The callback kvm_set_pic_irq() then blindly passes the newly-NULL pointer
to pch_pic_set_irq() without checking it:

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

Since pch_pic_set_irq() unconditionally dereferences the pointer:

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

Doesn't this mean an unprivileged userspace process exhausting file
descriptors and concurrently injecting interrupts will trigger a host panic?

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