Re: [PATCH 1/2] LoongArch: KVM: EIOINTC: clamp ipnum to valid range in INT_ENCODE mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Guest or host userspace can trigger massive out-of-bounds reads and writes on the host kernel heap and undefined bitshift behavior by supplying an unvalidated MSI data value.
- [High] `eiointc_set_sw_coreisr` fails to decode `cpuid` in 1-hot mode (`EIOINTC_ENABLE_CPU_ENCODE` disabled), reading from the wrong vCPU's `coreisr` state when reconstructing `sw_coreisr` during `LOAD_FINISHED`.
- [High] When a guest writes to `EIOINTC_COREISR_START` to clear a pending interrupt, `eiointc_update_irq` clears the fast-path state (`sw_coreisr`) and de-asserts the parent IRQ on the currently routed CPU from `sw_coremap`, which may not be the CPU whose `coreisr` was actually cleared.
- [Medium] Lockless access to `map->phys_map[cpuid]` in `kvm_get_vcpu_by_cpuid()` lacks memory barriers.
--

--- Patch [1]: [PATCH 1/2] LoongArch: KVM: EIOINTC: clamp ipnum to valid range in INT_ENCODE mode ---
commit 21ec11994b801fe6a14903dedc825bbe4a3246e5
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: EIOINTC: clamp ipnum to valid range in INT_ENCODE mode
    
This commit clamps the ipnum variable to [0, LOONGSON_IP_NUM) in INT_ENCODE
mode. It prevents out-of-bounds accesses on the sw_coreisr array which is
guest-programmable, preventing out-of-bounds accesses reachable from an
unprivileged guest.

Cc: stable@vger.kernel.org

> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> index 2b14485d14a70..0c34d7ab264dc 100644
> --- a/arch/loongarch/kvm/intc/eiointc.c
> +++ b/arch/loongarch/kvm/intc/eiointc.c
> @@ -17,6 +17,8 @@ static void eiointc_set_sw_coreisr(struct loongarch_eiointc *s)
>  		if (!(s->status & BIT(EIOINTC_ENABLE_INT_ENCODE))) {
>  			ipnum = count_trailing_zeros(ipnum);
>  			ipnum = ipnum < 4 ? ipnum : 0;
> +		} else {
> +			ipnum = (ipnum < LOONGSON_IP_NUM) ? ipnum : 0;
>  		}
>  
>  		cpuid = ((u8 *)s->coremap)[irq];

[Severity: High]
This is a pre-existing issue, but does eiointc_set_sw_coreisr() fail to
decode cpuid in 1-hot mode when EIOINTC_ENABLE_CPU_ENCODE is disabled?

It looks like it fetches cpuid directly from s->coremap[irq] without decoding
it. In the default 1-hot mode, cpuid represents a bitmask, but the subsequent
call to kvm_get_vcpu_by_cpuid() expects an exact ID. Could this result in
looking up the wrong vCPU during state restoration, causing lost interrupts?

[Severity: Medium]
This is a pre-existing issue, but is the lockless access to
map->phys_map[cpuid] safe in kvm_get_vcpu_by_cpuid() when called here?

When kvm_get_vcpu_by_cpuid() reads map->phys_map[cpuid].enabled and vcpu, it
doesn't use memory barriers like smp_load_acquire(). Concurrently,
kvm_set_cpuid() updates these fields using a lock but without
smp_store_release(). Could this compiler or CPU reordering cause
kvm_get_vcpu_by_cpuid() to transiently return NULL?

[ ... ]

> @@ -42,6 +44,8 @@ static void eiointc_update_irq(struct loongarch_eiointc *s, int irq, int level)
>  	if (!(s->status & BIT(EIOINTC_ENABLE_INT_ENCODE))) {
>  		ipnum = count_trailing_zeros(ipnum);
>  		ipnum = ipnum < 4 ? ipnum : 0;
> +	} else {
> +		ipnum = (ipnum < LOONGSON_IP_NUM) ? ipnum : 0;
>  	}
>  
>  	cpu = s->sw_coremap[irq];

[Severity: Critical]
This is a pre-existing issue, but is the irq index validated before being
used for array accesses here?

The KVM MSI routing mechanism allows guests or host userspace to inject MSIs
with an arbitrary 32-bit msi.data payload. In pch_msi_set_irq():

arch/loongarch/kvm/intc/pch_pic.c:pch_msi_set_irq() {
    ...
    eiointc_set_irq(kvm->arch.eiointc, e->msi.data, level);
    ...
}

The user-supplied e->msi.data is passed unvalidated to eiointc_set_irq() as
the irq parameter. This unvalidated irq is then used as an index for bitwise
operations like __set_bit(irq, isr) in eiointc_set_irq() and array accesses
like s->sw_coremap[irq] in eiointc_update_irq(). Can this allow unprivileged
guest code to perform arbitrary out-of-bounds memory writes on the host
kernel heap?

[Severity: High]
This isn't a bug introduced by this patch, but does eiointc_update_irq()
operate on the correct CPU when clearing a pending interrupt?

When a guest writes to EIOINTC_COREISR_START to clear a pending interrupt:

arch/loongarch/kvm/intc/eiointc.c:loongarch_eiointc_write() {
    ...
    eiointc_update_irq(s, irq + index * 64, 0);
    ...
}

The write handler issues the clear request without explicitly passing the
original vCPU. Then eiointc_update_irq() dynamically fetches the target CPU
from s->sw_coremap[irq]. If the guest has re-routed the IRQ while it was
pending, sw_coremap will point to the new CPU. Will this clear the fast-path
sw_coreisr state on the new CPU, leaving the old CPU with a permanently
asserted parent IRQ and an unrecoverable interrupt storm?

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