Re: [PATCH 3/6] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: missing READ_ONCE() allows TOCTOU NULL dereference in MSI routing
--

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

LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

This commit attempts to fix a time-of-check to time-of-use race
condition by loading the kvm->arch.dmsintc pointer into a local variable
once before checking it. This is intended to prevent a concurrent device
removal from causing a NULL pointer dereference.

> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> index 194b9f748550e..802c8f9920935 100644
> --- a/arch/loongarch/kvm/intc/dmsintc.c
> +++ b/arch/loongarch/kvm/intc/dmsintc.c
> @@ -70,11 +70,13 @@ int dmsintc_set_irq(struct kvm *kvm, u64 addr, int data, int level)
>  	unsigned int irq, cpu;
>  	struct kvm_vcpu *vcpu;
>  
> -	if (!kvm->arch.dmsintc)
> +	struct loongarch_dmsintc *s = kvm->arch.dmsintc;

[Severity: High]
Does this assignment prevent the C compiler from reloading the pointer from
memory? Without a READ_ONCE() wrapper around kvm->arch.dmsintc, the compiler
is permitted to optimize away the local storage and emit multiple loads from
the shared memory location. This could reintroduce the exact race condition
and NULL pointer dereference that this patch is trying to fix.

> +
> +	if (!s)
>  		return -EINVAL;
>  
>  	irq = (addr >> AVEC_IRQ_SHIFT) & AVEC_IRQ_MASK;
> -	cpu = (addr >> AVEC_CPU_SHIFT) & kvm->arch.dmsintc->cpu_mask;
> +	cpu = (addr >> AVEC_CPU_SHIFT) & s->cpu_mask;
>  	if (cpu >= KVM_MAX_VCPUS)
>  		return -EINVAL;

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> index 2ac207271bab1..3517dcc42d22d 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -74,10 +74,11 @@ void pch_pic_set_irq(struct loongarch_pch_pic *s, int irq, int level)
>  int pch_msi_set_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, int level)
>  {
>  	u64 msg_addr = (((u64)e->msi.address_hi) << 32) | e->msi.address_lo;
> +	struct loongarch_dmsintc *dmsintc = kvm->arch.dmsintc;

[Severity: High]
Could the compiler similarly reload kvm->arch.dmsintc here since it lacks a
READ_ONCE() wrapper? It seems like multiple memory accesses could still occur
during the address-window comparison below, exposing the vulnerability
during concurrent device removal.

>  
> -	if (cpu_has_msgint && kvm->arch.dmsintc &&
> -		msg_addr >= kvm->arch.dmsintc->msg_addr_base &&
> -		msg_addr < (kvm->arch.dmsintc->msg_addr_base + kvm->arch.dmsintc->msg_addr_size)) {
> +	if (cpu_has_msgint && dmsintc &&
> +		msg_addr >= dmsintc->msg_addr_base &&
> +		msg_addr < (dmsintc->msg_addr_base + dmsintc->msg_addr_size)) {
>  		return dmsintc_set_irq(kvm, msg_addr, e->msi.data, level);
>  	}

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