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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] kvm: UAF window in dmsintc due to kfree before pointer clear
- [High] kvm: TOCTOU UAF and NULL deref in lockless IRQ injection
- [High] kvm: loongarch: unsynchronized device teardown leads to NULL pointer dereference in irq injection
--

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

LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

This commit aims to fix a dangling pointer vulnerability by explicitly
clearing device pointers in the destroy callbacks for LoongArch KVM irqchip
devices.

> 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: High]
Since the pointer is explicitly freed before it is set to NULL, does this
leave a race window for concurrent lockless readers?

If anon_inode_getfd() fails during device creation, this destroy callback
runs while the VM is still active. A concurrent interrupt injection could
read the pointer right after the kfree() but before it is cleared to NULL,
leading to a use-after-free.

>  	kfree(dev);
>  }
>  

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> index 84d84bd432d74..9a8498c5deb9d 100644
> --- 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);
>  	mutex_unlock(&kvm->slots_lock);
>  	kfree(eiointc);

[Severity: High]
Could clearing the pointer and freeing the device here without SRCU
synchronization lead to a use-after-free or NULL pointer dereference in
lockless IRQ injection paths?

If a concurrent thread is injecting interrupts (e.g., via KVM_IRQ_LINE) while
the device is being destroyed, it drops kvm->irq_srcu and locklessly calls
pch_msi_set_irq() or similar helpers. If it evaluates
kvm_arch_irqchip_in_kernel() before the pointer is cleared, it might pass
the dangling eiointc pointer to eiointc_set_irq(), which then blindly
dereferences it. Without a call to synchronize_srcu(&kvm->irq_srcu) before
freeing, this appears to result in a use-after-free.

Similarly, if the injection path reads the pointer exactly after it is set
to NULL, it passes a NULL pointer to eiointc_set_irq(), which will crash
when calling spin_lock_irqsave(&s->lock, flags).

>  	kfree(dev);
>  }

[ ... ]

> 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;
>  	/* 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);
>  	mutex_unlock(&kvm->slots_lock);
>  	kfree(s);

[Severity: High]
Does unsynchronized device teardown here lead to a similar NULL pointer
dereference in the IRQ injection path?

If KVM_IRQFD or KVM_IRQ_LINE is called concurrently, kvm_set_irq() looks up
the routing and calls kvm_set_pic_irq(). The helper unconditionally reads
kvm->arch.pch_pic and passes it to 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);
        ...
}

If the pointer was just cleared to NULL by this destroy callback, it results
in a deterministic NULL pointer dereference when pch_pic_set_irq()
immediately calls spin_lock(&s->lock). Should there be synchronization
against concurrent IRQ routing here?

>  	kfree(dev);
>  }

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