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

[sashiko-bot@kernel.org]

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