Re: [PATCH v3 4/9] LoongArch: KVM: INTC: Check validation of num_cpu from user space

On Fri, Jun 20, 2025 at 9:43 AM Bibo Mao maobibo@loongson.cn wrote:

On 2025/6/19 下午4:46, Huacai Chen wrote:

...

Hi, Bibo,

On Wed, Jun 11, 2025 at 9:47 AM Bibo Mao maobibo@loongson.cn wrote:

...

The maximum supported cpu number is EIOINTC_ROUTE_MAX_VCPUS about irqchip eiointc, here add validation about cpu number to avoid array pointer overflow.

Cc: stable@vger.kernel.org Fixes: 1ad7efa552fd ("LoongArch: KVM: Add EIOINTC user mode read and write functions") Signed-off-by: Bibo Mao maobibo@loongson.cn

---

arch/loongarch/kvm/intc/eiointc.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c index b48511f903b5..ed80bf290755 100644 --- a/arch/loongarch/kvm/intc/eiointc.c +++ b/arch/loongarch/kvm/intc/eiointc.c @@ -798,7 +798,7 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev, int ret = 0; unsigned long flags; unsigned long type = (unsigned long)attr->attr;

•   u32 i, start_irq;
  

•   u32 i, start_irq, val;
     void __user *data;
     struct loongarch_eiointc *s = dev->kvm->arch.eiointc;
  
  

@@ -806,7 +806,12 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev, spin_lock_irqsave(&s->lock, flags); switch (type) { case KVM_DEV_LOONGARCH_EXTIOI_CTRL_INIT_NUM_CPU:

•           if (copy_from_user(&s->num_cpu, data, 4))
  

•           if (copy_from_user(&val, data, 4) == 0) {
  

•                   if (val < EIOINTC_ROUTE_MAX_VCPUS)
  

•                           s->num_cpu = val;
  

•                   else
  

•                           ret = -EINVAL;
  

Maybe it is better to set s->num_cpu to EIOINTC_ROUTE_MAX_VCPUS (or other value) rather than keep it uninitialized. Because in other places we need to check s->num_cpu and an uninitialized value may cause undefined behavior.

There is error return value -EINVAL, VMM should stop running and exit immediately if there is error return value with the ioctl command.

num_cpu is not uninitialized and it is zero by default. If VMM does not care about the return value, VMM will fail to get coreisr information in future.

If you are sure you can keep it as is. Then please resend patch 1,2,3,4,5,9 as a series because they are all bug fixes that should be merged as soon as possible. And in my own opinion, "INTC" can be dropped in the title.

Huacai

Regards Bibo Mao

...

Huacai

...

•           } else
                     ret = -EFAULT;
             break;
     case KVM_DEV_LOONGARCH_EXTIOI_CTRL_INIT_FEATURE:
  

@@ -835,7 +840,7 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev, struct kvm_device_attr *attr, bool is_write) {

•   int addr, cpuid, offset, ret = 0;
  

•   int addr, cpu, offset, ret = 0;
     unsigned long flags;
     void *p = NULL;
     void __user *data;
  

@@ -843,7 +848,7 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,

     s = dev->kvm->arch.eiointc;
     addr = attr->attr;

•   cpuid = addr >> 16;
  

•   cpu = addr >> 16;
     addr &= 0xffff;
     data = (void __user *)attr->addr;
     switch (addr) {
  

@@ -868,8 +873,11 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev, p = &s->isr.reg_u32[offset]; break; case EIOINTC_COREISR_START ... EIOINTC_COREISR_END:

•           if (cpu >= s->num_cpu)
  

•                   return -EINVAL;
  

•            offset = (addr - EIOINTC_COREISR_START) / 4;
  

•           p = &s->coreisr.reg_u32[cpuid][offset];
  

•           p = &s->coreisr.reg_u32[cpu][offset];
             break;
     case EIOINTC_COREMAP_START ... EIOINTC_COREMAP_END:
             offset = (addr - EIOINTC_COREMAP_START) / 4;
  

-- 2.39.3