linux-kernel - Re: [PATCH v3 4/9] LoongArch: KVM: INTC: Check validation of num

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f1b9068-2d3d-2f89-4f72-85b021537f58@loongson.cn>
Date: Fri, 20 Jun 2025 09:42:00 +0800
From: Bibo Mao <maobibo@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Tianrui Zhao <zhaotianrui@...ngson.cn>,
 Xianglai Li <lixianglai@...ngson.cn>, kvm@...r.kernel.org,
 loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH v3 4/9] LoongArch: KVM: INTC: Check validation of num_cpu
 from user space



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

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
>>