[PATCH] LoongArch: KVM: User internal API to deliver interrupt in kernel mode

[PATCH] LoongArch: KVM: User internal API to deliver interrupt in kernel mode

[Bibo Mao]

API kvm_vcpu_ioctl_interrupt() is mainly to used to deliver interrupt
from user mode, and internal APIs kvm_queue_irq() and kvm_dequeue_irq()
are used in kernel mode.

Also this patch removes unnecessary printk information with inject
unsupported interrupt with number 0.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/kvm/intc/dmsintc.c |  7 ++++---
 arch/loongarch/kvm/intc/eiointc.c |  8 +++++---
 arch/loongarch/kvm/intc/ipi.c     | 10 ++++------
 arch/loongarch/kvm/vcpu.c         |  4 +---
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
index de25735ce039..a726c6cafec3 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)
 int dmsintc_deliver_msi_to_vcpu(struct kvm *kvm,
 				struct kvm_vcpu *vcpu, u32 vector, int level)
 {
-	struct kvm_interrupt vcpu_irq;
 	struct dmsintc_state *ds = &vcpu->arch.dmsintc_state;
 
 	if (!level)
@@ -57,9 +56,11 @@ int dmsintc_deliver_msi_to_vcpu(struct kvm *kvm,
 	if (!ds)
 		return -ENODEV;
 
-	vcpu_irq.irq = INT_AVEC;
+	if (!kvm_guest_has_msgint(&vcpu->arch))
+		return -EINVAL;
+
 	set_bit(vector, (unsigned long *)&ds->vector_map);
-	kvm_vcpu_ioctl_interrupt(vcpu, &vcpu_irq);
+	kvm_queue_irq(vcpu, INT_AVEC);
 	kvm_vcpu_kick(vcpu);
 
 	return 0;
diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
index 2ab7fafa86d5..63f917de7903 100644
--- a/arch/loongarch/kvm/intc/eiointc.c
+++ b/arch/loongarch/kvm/intc/eiointc.c
@@ -36,7 +36,6 @@ static void eiointc_update_irq(struct loongarch_eiointc *s, int irq, int level)
 {
 	int ipnum, cpu, found;
 	struct kvm_vcpu *vcpu;
-	struct kvm_interrupt vcpu_irq;
 
 	ipnum = (s->ipmap >> (irq / 32 * 8)) & 0xff;
 	if (!(s->status & BIT(EIOINTC_ENABLE_INT_ENCODE))) {
@@ -67,8 +66,11 @@ static void eiointc_update_irq(struct loongarch_eiointc *s, int irq, int level)
 	if (found < EIOINTC_IRQS)
 		return; /* other irq is handling, needn't update parent irq */
 
-	vcpu_irq.irq = level ? (INT_HWI0 + ipnum) : -(INT_HWI0 + ipnum);
-	kvm_vcpu_ioctl_interrupt(vcpu, &vcpu_irq);
+	if (level)
+		kvm_queue_irq(vcpu, INT_HWI0 + ipnum);
+	else
+		kvm_dequeue_irq(vcpu, INT_HWI0 + ipnum);
+	kvm_vcpu_kick(vcpu);
 }
 
 static inline void eiointc_update_sw_coremap(struct loongarch_eiointc *s,
diff --git a/arch/loongarch/kvm/intc/ipi.c b/arch/loongarch/kvm/intc/ipi.c
index 1f6ebbd0af5c..ab3d894eb146 100644
--- a/arch/loongarch/kvm/intc/ipi.c
+++ b/arch/loongarch/kvm/intc/ipi.c
@@ -10,15 +10,14 @@
 static void ipi_set(struct kvm_vcpu *vcpu, uint32_t data)
 {
 	uint32_t status;
-	struct kvm_interrupt irq;
 
 	spin_lock(&vcpu->arch.ipi_state.lock);
 	status = vcpu->arch.ipi_state.status;
 	vcpu->arch.ipi_state.status |= data;
 	spin_unlock(&vcpu->arch.ipi_state.lock);
 	if ((status == 0) && data) {
-		irq.irq = LARCH_INT_IPI;
-		kvm_vcpu_ioctl_interrupt(vcpu, &irq);
+		kvm_queue_irq(vcpu, LARCH_INT_IPI);
+		kvm_vcpu_kick(vcpu);
 	}
 }
 
@@ -40,15 +39,14 @@ static void ipi_send(struct kvm *kvm, uint64_t data)
 static void ipi_clear(struct kvm_vcpu *vcpu, uint64_t data)
 {
 	uint32_t status;
-	struct kvm_interrupt irq;
 
 	spin_lock(&vcpu->arch.ipi_state.lock);
 	vcpu->arch.ipi_state.status &= ~data;
 	status = vcpu->arch.ipi_state.status;
 	spin_unlock(&vcpu->arch.ipi_state.lock);
 	if (status == 0) {
-		irq.irq = -LARCH_INT_IPI;
-		kvm_vcpu_ioctl_interrupt(vcpu, &irq);
+		kvm_dequeue_irq(vcpu, LARCH_INT_IPI);
+		kvm_vcpu_kick(vcpu);
 	}
 }
 
diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index e28084c49e68..bd90be3f498b 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -1492,10 +1492,8 @@ int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, struct kvm_interrupt *irq)
 		kvm_queue_irq(vcpu, intr);
 	else if (intr < 0)
 		kvm_dequeue_irq(vcpu, -intr);
-	else {
-		kvm_err("%s: invalid interrupt ioctl %d\n", __func__, irq->irq);
+	else
 		return -EINVAL;
-	}
 
 	kvm_vcpu_kick(vcpu);
 

base-commit: 0e0611827f3349d0a2ac121c023a6d3260dcecdb
-- 
2.39.3

Re: [PATCH] LoongArch: KVM: User internal API to deliver interrupt in kernel mode

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The KVM_INTERRUPT ioctl lacks bounds checking on the user-provided interrupt number, allowing unprivileged host userspace to trigger out-of-bounds bit operations.
- [Critical] Guest-controlled interrupt pin number extraction allows out-of-bounds bitwise writes in host kernel memory.
- [High] Time-of-check to time-of-use (TOCTOU) race condition in IPI delivery and clearance allows inter-processor interrupts to be lost.
--

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

LoongArch: KVM: User internal API to deliver interrupt in kernel mode

This commit refactors interrupt delivery in kernel mode to use the internal
APIs kvm_queue_irq() and kvm_dequeue_irq() directly, bypassing the ioctl
interface. It also removes the printk error message for invalid interrupts
with number 0.

> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> index 2ab7fafa86d59..63f917de7903c 100644
> --- a/arch/loongarch/kvm/intc/eiointc.c
> +++ b/arch/loongarch/kvm/intc/eiointc.c
[ ... ]
> @@ -67,8 +66,11 @@ static void eiointc_update_irq(struct loongarch_eiointc *s, int irq, int level)
>  	if (found < EIOINTC_IRQS)
>  		return; /* other irq is handling, needn't update parent irq */
>  
> -	vcpu_irq.irq = level ? (INT_HWI0 + ipnum) : -(INT_HWI0 + ipnum);
> -	kvm_vcpu_ioctl_interrupt(vcpu, &vcpu_irq);
> +	if (level)
> +		kvm_queue_irq(vcpu, INT_HWI0 + ipnum);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the guest-controlled
ipnum lack bounds checking before being used here?

In eiointc_update_irq(), ipnum is calculated from the guest-controlled
s->ipmap register. If the guest enables EIOINTC_ENABLE_INT_ENCODE, this
value can be passed unmodified.

Since INT_HWI0 is 2, the passed IRQ number can be up to 257. Because
kvm_queue_irq() uses this to set a bit in vcpu->arch.irq_pending (which
is a single 64-bit unsigned long), does this write past the end of
irq_pending and corrupt adjacent host memory?

> +	else
> +		kvm_dequeue_irq(vcpu, INT_HWI0 + ipnum);
> +	kvm_vcpu_kick(vcpu);
>  }

> diff --git a/arch/loongarch/kvm/intc/ipi.c b/arch/loongarch/kvm/intc/ipi.c
> index 1f6ebbd0af5c8..ab3d894eb1469 100644
> --- a/arch/loongarch/kvm/intc/ipi.c
> +++ b/arch/loongarch/kvm/intc/ipi.c
[ ... ]
> @@ -10,15 +10,14 @@
>  static void ipi_set(struct kvm_vcpu *vcpu, uint32_t data)
>  {
>  	uint32_t status;
> -	struct kvm_interrupt irq;
>  
>  	spin_lock(&vcpu->arch.ipi_state.lock);
>  	status = vcpu->arch.ipi_state.status;
>  	vcpu->arch.ipi_state.status |= data;
>  	spin_unlock(&vcpu->arch.ipi_state.lock);
>  	if ((status == 0) && data) {
> -		irq.irq = LARCH_INT_IPI;
> -		kvm_vcpu_ioctl_interrupt(vcpu, &irq);
> +		kvm_queue_irq(vcpu, LARCH_INT_IPI);

[Severity: High]
This isn't a bug introduced by this patch, but can a race condition here
cause inter-processor interrupts to be lost?

The queuing of the interrupt via kvm_queue_irq() happens outside the lock
based on a local snapshot of status. If CPU 0 clears an IPI and unlocks, and
CPU 1 sets a new IPI and queues it, could CPU 0 then resume and incorrectly
execute kvm_dequeue_irq() based on its stale status?

> +		kvm_vcpu_kick(vcpu);
>  	}
>  }
>  
[ ... ]
> @@ -40,15 +39,14 @@ static void ipi_send(struct kvm *kvm, uint64_t data)
>  static void ipi_clear(struct kvm_vcpu *vcpu, uint64_t data)
>  {
>  	uint32_t status;
> -	struct kvm_interrupt irq;
>  
>  	spin_lock(&vcpu->arch.ipi_state.lock);
>  	vcpu->arch.ipi_state.status &= ~data;
>  	status = vcpu->arch.ipi_state.status;
>  	spin_unlock(&vcpu->arch.ipi_state.lock);
>  	if (status == 0) {
> -		irq.irq = -LARCH_INT_IPI;
> -		kvm_vcpu_ioctl_interrupt(vcpu, &irq);
> +		kvm_dequeue_irq(vcpu, LARCH_INT_IPI);

[Severity: High]
This isn't a bug introduced by this patch, but this dequeue operation appears
to mirror the race condition mentioned above, potentially clearing an interrupt
that was just queued by another CPU.

> +		kvm_vcpu_kick(vcpu);
>  	}
>  }

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index e28084c49e682..bd90be3f498b4 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
[ ... ]
> @@ -1492,10 +1492,8 @@ int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, struct kvm_interrupt *irq)
>  		kvm_queue_irq(vcpu, intr);
>  	else if (intr < 0)
>  		kvm_dequeue_irq(vcpu, -intr);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the KVM_INTERRUPT ioctl
handler lack bounds checking on the user-provided interrupt number?

The ioctl parses an __u32 irq from user memory and passes it to
kvm_vcpu_ioctl_interrupt(), which unconditionally passes it to
kvm_queue_irq() or kvm_dequeue_irq().

Since these functions execute atomic bit operations (set_bit/clear_bit) on
vcpu->arch.irq_pending and vcpu->arch.irq_clear (which are single 64-bit
unsigned long values), could an unprivileged host process with /dev/kvm
access supply arbitrarily large values to manipulate host kernel memory
outside the bounds of the VCPU architecture struct?

> -	else {
> -		kvm_err("%s: invalid interrupt ioctl %d\n", __func__, irq->irq);
> +	else
>  		return -EINVAL;
> -	}
>  
>  	kvm_vcpu_kick(vcpu);
>

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