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

[Huacai Chen]

On Fri, Jun 20, 2025 at 9:43 AM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2025/6/19 下午4:46, Huacai Chen wrote:
> > Hi, Bibo,
> >
> > On Wed, Jun 11, 2025 at 9:47 AM Bibo Mao <maobibo@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> >> Fixes: 1ad7efa552fd ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
> >> Signed-off-by: Bibo Mao <maobibo@xxxxxxxxxxx>
> >> ---
> >>   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
> >>
>