Re: [PATCH v3 3/3] LoongArch: KVM: Use internal API to deliver interrupt in kernel mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Multiple pre-existing WARN_ONCE and WARN_ON_ONCE macros are reachable by unprivileged host userspace and malicious guests, allowing a host-side DoS if `panic_on_warn` is enabled.
- [High] Uninitialized array `vector` is conditionally assigned but unconditionally evaluated and used to modify hardware CSRs.
--

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

LoongArch: KVM: Use internal API to deliver interrupt in kernel mode
    
This commit updates KVM on LoongArch to use internal APIs
kvm_queue_irq() and kvm_dequeue_irq() for delivering interrupts in kernel
mode, rather than the user mode kvm_vcpu_ioctl_interrupt(). It also moves
IPI injection and ack operations within lock protection to reduce contention.

> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> index 708abbcd55bff..ed85532247759 100644
> --- a/arch/loongarch/kvm/intc/dmsintc.c
> +++ b/arch/loongarch/kvm/intc/dmsintc.c
> @@ -47,7 +47,6 @@ void dmsintc_inject_irq(struct kvm_vcpu *vcpu)

[Severity: High]
This isn't a bug introduced by this patch, but looking at dmsintc_inject_irq()
just above this change, is the vector array properly initialized?

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]) {
                old = kvm_read_hw_gcsr(LOONGARCH_CSR_ISR0);
                kvm_write_hw_gcsr(LOONGARCH_CSR_ISR0, vector[0] | old);
        }
}

If atomic64_read() returns zero, vector[i] is left uninitialized. When it
checks if (vector[0]) later, could this evaluate to true using stack garbage
and write random data into the guest CSRs?

>  int dmsintc_deliver_msi_to_vcpu(struct kvm *kvm,
>  				struct kvm_vcpu *vcpu, u32 vector, int level)
>  {

[ ... ]

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 361fca46e2b32..265aaad633157 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -1465,6 +1465,9 @@ int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, struct kvm_interrupt *irq)
>  	if (vector >= EXCCODE_INT_NUM)
>  		return -EINVAL;
>  
> +	if (kvm_arch_irqchip_in_kernel(vcpu->kvm))
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but I noticed there are several WARN_ONCE and
WARN_ON_ONCE macros reachable by unprivileged host userspace and malicious
guests in this subsystem.

For example, in kvm_vcpu_ioctl_interrupt(), providing an intr == 0
unconditionally hits a fallback block that calls:

WARN_ONCE(1, "%s: invalid interrupt ioctl %d\n", __func__, irq->irq);

Similarly, in ipi_send() and eiointc_update_irq(), guest-controlled CPU IDs
can yield a NULL vCPU, directly triggering:

WARN_ONCE(1, "%s: invalid target cpu: %d\n", __func__, cpu);

And in loongarch_ipi_readl(), a guest unaligned MMIO read triggers:

WARN_ON_ONCE(offset & (len - 1));

Could these be converted to pr_warn_once(), return an error, or drop the
assertions entirely? These could allow a host-side denial of service if
panic_on_warn is enabled.

>  	if (!kvm_guest_has_msgint(&vcpu->arch) && (vector == INT_AVEC))
>  		return -EINVAL;
>

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