[PATCH] LoongArch: KVM: Initialize vector array in dmsintc_inject_irq()

[PATCH] LoongArch: KVM: Initialize vector array in dmsintc_inject_irq()

[Naveed Khan]

dmsintc_inject_irq() declares the on-stack array 'vector[4]' without an
initializer and only assigns an element when the corresponding
ds->vector_map[] word is non-zero:

	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);
	}

It then reads all four elements unconditionally to decide whether to OR
them into the guest CSR registers LOONGARCH_CSR_ISR0..ISR3:

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

When a vector_map word is zero, the matching vector[] element is left
uninitialized. Its stale stack contents are then written into the
guest-visible ISR registers, both leaking kernel stack data to the guest
(which can read the ISR CSRs) and injecting spurious interrupt vectors.

The path is guest reachable: a guest-directed MSI sets bits in
vector_map via dmsintc_deliver_msi_to_vcpu(), and AVEC delivery
(kvm_irq_deliver() -> dmsintc_inject_irq()) then consumes the
partially-initialized array. In the common case where the guest only
uses low vectors, only vector_map[0] is set and vector[1..3] are read
uninitialized on every injection.

Zero-initialize the array so that words with no pending vectors are
skipped as intended.

Signed-off-by: Naveed Khan <naveed@digiscrypt.com>
---
diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
index de25735ce0..ef6c064e52 100644
--- a/arch/loongarch/kvm/intc/dmsintc.c
+++ b/arch/loongarch/kvm/intc/dmsintc.c
@@ -11,7 +11,7 @@
 void dmsintc_inject_irq(struct kvm_vcpu *vcpu)
 {
 	unsigned int i;
-	unsigned long vector[4], old;
+	unsigned long vector[4] = {}, old;
 	struct dmsintc_state *ds = &vcpu->arch.dmsintc_state;
 
 	if (!ds)
-- 
2.52.0

Re: [PATCH] LoongArch: KVM: Initialize vector array in dmsintc_inject_irq()

[sashiko-bot]

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

Pre-existing issues:
- [High] Undefined behavior from passing a negative/underflowed `high` index to `GENMASK`.
- [High] Missing memory barriers between writing/reading interrupt data and the pending flag causes lost MSIs.
- [High] TOCTOU race in KVM_SET_DEVICE_ATTR ioctl handler allows duplicate or inconsistent device configuration.
--

--- Patch [1]: [PATCH] LoongArch: KVM: Initialize vector array in dmsintc_inject_irq() ---
commit 83b5de435c7563536835859a673b789e3b32120a
Author: Naveed Khan <naveed@digiscrypt.com>

LoongArch: KVM: Initialize vector array in dmsintc_inject_irq()

This commit initializes the vector array in dmsintc_inject_irq() to prevent
uninitialized stack data from being written to guest-visible ISR registers.
Previously, vector elements were left uninitialized when their corresponding
vector_map word was zero, which could leak kernel stack data or inject spurious
interrupts.

> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> index de25735ce0394..ef6c064e52f18 100644
> --- a/arch/loongarch/kvm/intc/dmsintc.c
> +++ b/arch/loongarch/kvm/intc/dmsintc.c
> @@ -11,7 +11,7 @@
>  void dmsintc_inject_irq(struct kvm_vcpu *vcpu)
>  {
>  	unsigned int i;
> -	unsigned long vector[4], old;
> +	unsigned long vector[4] = {}, old;
>  	struct dmsintc_state *ds = &vcpu->arch.dmsintc_state;
>  
>  	if (!ds)

While reviewing this, I noticed a few pre-existing issues in the KVM interrupt
handling code.

arch/loongarch/kvm/intc/dmsintc.c:kvm_dmsintc_ctrl_access() {
...
                if (is_write) {
                        if (copy_from_user(&val, data, sizeof(s->msg_addr_base)))
                                return -EFAULT;
                        if (s->msg_addr_base)
                                return -EFAULT; /* Duplicate setting are not allowed. */
                        if ((val & (BIT(AVEC_CPU_SHIFT) - 1)) != 0)
                                return -EINVAL;
                        s->msg_addr_base = val;
...

[Severity: High]
This is a pre-existing issue, but is there a time-of-check to time-of-use race
here?

Device attributes like msg_addr_base are updated without holding a lock. If
multiple VMM threads concurrently configure the device, couldn't they both
pass the if (s->msg_addr_base) check before either thread writes to it?

Would this bypass the duplicate setting check, and could interleaved writes
lead to a corrupted device state where msg_addr_base and cpu_mask become
inconsistent?

...
                        s->msg_addr_base = val;
                        cpu_bit = find_first_bit((unsigned long *)&(s->msg_addr_base), 64) - AVEC_CPU_SHIFT;
                        cpu_bit = min(cpu_bit, AVEC_CPU_BIT);
                        s->cpu_mask = GENMASK(cpu_bit - 1, 0) & AVEC_CPU_MASK;
...

[Severity: High]
This is a pre-existing issue, but does this code risk undefined behavior?

If userspace configures the device with a msg_addr_base where the first set
bit is exactly at AVEC_CPU_SHIFT, find_first_bit() would cause cpu_bit to
become 0.

The macro GENMASK(cpu_bit - 1, 0) would then evaluate to
GENMASK(ULONG_MAX, 0).

Doesn't this shift by the width of the type, causing undefined behavior
that could set s->cpu_mask to AVEC_CPU_MASK instead of 0?


I also noticed a potential issue in the interrupt delivery paths:

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);
        kvm_vcpu_kick(vcpu);
...
}

arch/loongarch/kvm/interrupt.c:kvm_deliver_intr() {
...
        mask = READ_ONCE(vcpu->arch.irq_pending);
        if (mask) {
                mask = xchg_relaxed(&vcpu->arch.irq_pending, 0);
                kvm_irq_deliver(vcpu, mask);
        }
...
}

[Severity: High]
This is a pre-existing issue, but are memory barriers needed here?

The sender (dmsintc_deliver_msi_to_vcpu) signals an interrupt by setting a
bit in ds->vector_map and then sets irq_pending via
kvm_vcpu_ioctl_interrupt(). Since both use set_bit(), which provides no
memory ordering, the writes could be reordered.

The receiver (kvm_deliver_intr) reads irq_pending, clears it, and then
reads vector_map.

Without memory barriers (like smp_wmb() on the sender and smp_rmb() on the
receiver), could the receiver see irq_pending set but read a stale 0 from
vector_map, causing it to clear the pending flag without injecting the
vector?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178430657065.19695.6391637564764566917@digiscrypt.com?part=1