Re: [PATCH v6 2/6] LoongArch: KVM: Check msgint feature in interrupt post

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Uninitialized variable `vector` array leads to host stack data leakage and guest state corruption.
- [High] Missing memory barriers between writing the MSI vector map and setting the interrupt pending flag causes lost interrupts.
--

commit 7cbebc4f6556c5cec6e10f7b92d02d8b3f478b95
Author: Bibo Mao <maobibo@loongson.cn>

LoongArch: KVM: Check msgint feature in interrupt post

Interrupt AVEC is valid only if VM has msgint feature, and this feature
is checked in interrupt handling. Since interrupt handling is executing
in VM context switch, and it is hot path, here move the feature checking
in interrupt post rather than interrupt handling.

> diff --git a/arch/loongarch/kvm/interrupt.c b/arch/loongarch/kvm/interrupt.c
> index a18c60dffbbae..48dd56aa4dc56 100644
> --- a/arch/loongarch/kvm/interrupt.c
> +++ b/arch/loongarch/kvm/interrupt.c
> @@ -36,8 +36,6 @@ static int kvm_irq_deliver(struct kvm_vcpu *vcpu, unsigned int priority)
>  
>  	switch (priority) {
>  	case INT_AVEC:
> -		if (!kvm_guest_has_msgint(&vcpu->arch))
> -			break;
>  		dmsintc_inject_irq(vcpu);

[Severity: Critical]
This is a pre-existing issue, but in dmsintc_inject_irq() the vector array
is declared uninitialized:

arch/loongarch/kvm/intc/dmsintc.c:dmsintc_inject_irq() {
	unsigned long vector[4], old;
	...
	for (i = 0; i < 4; i++) {
		old = atomic64_read(&(ds->vector_map[i]));
		if (old)
			vector[i] = atomic64_xchg(&(ds->vector_map[i]), 0);
	}

	if (vector[0]) {
		...
}

If vector_map[i] is zero, vector[i] retains uninitialized stack memory.
Because the check if (vector[i]) evaluates this garbage, could this
unconditionally write uninitialized host stack data into the guest's ISR
registers and corrupt the guest's interrupt state?

>  		fallthrough;
>  	case INT_TI:

[ ... ]

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index df5be9b265e8d..e986146d22724 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -1493,6 +1493,9 @@ int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, struct kvm_interrupt *irq)
>  	if (vector >= EXCCODE_INT_NUM)
>  		return -EINVAL;
>  
> +	if (!kvm_guest_has_msgint(&vcpu->arch) && (vector == INT_AVEC))
> +		return -EINVAL;
> +
>  	if (intr > 0)
>  		kvm_queue_irq(vcpu, intr);

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing memory
barrier in the producer/consumer paths for MSI injection?

In dmsintc_deliver_msi_to_vcpu(), we update the vector map and then queue
the interrupt notification without a write barrier:

arch/loongarch/kvm/intc/dmsintc.c:dmsintc_deliver_msi_to_vcpu() {
	...
	set_bit(vector, (unsigned long *)&ds->vector_map);
	kvm_vcpu_ioctl_interrupt(vcpu, &vcpu_irq); // calls kvm_queue_irq()
}

On the consumer side, kvm_deliver_intr() reads irq_pending and calls
kvm_irq_deliver(), which clears the pending bit and reads the vector map:

arch/loongarch/kvm/interrupt.c:kvm_deliver_intr() {
	...
	for_each_set_bit(priority, pending, EXCCODE_INT_NUM)
		kvm_irq_deliver(vcpu, priority);
}

Since set_bit() doesn't imply a memory barrier, could the CPU reorder the
writes, causing the consumer to see a pending interrupt but an empty
vector_map, leading to permanently lost MSI interrupts?

>  	else if (intr < 0)
>  		kvm_dequeue_irq(vcpu, -intr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611065815.1439154-1-maobibo@loongson.cn?part=2