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: <d197255b-9165-adc5-8ba1-a6d96579fc38@loongson.cn>
Date: Fri, 27 Jun 2025 15:42:57 +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/20 下午10:43, Huacai Chen wrote:
> On Fri, Jun 20, 2025 at 9:43 AM Bibo Mao <maobibo@...ngson.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@...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.
> 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.
Ok, will do in this way.

Regards
Bibo Mao
> 
> 
> 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
>>>>
>>