[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAAhV-H6bqQHQfVn7xiiMU5mxWSpMjPzs2JiJcgkuzFdEPEqUtw@mail.gmail.com>
Date: Fri, 27 Jun 2025 17:04:48 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Bibo Mao <maobibo@...ngson.cn>
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 Fri, Jun 27, 2025 at 3:44 PM Bibo Mao <maobibo@...ngson.cn> wrote:
>
>
>
> 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.
Not needed now, patches have been applied.
Huacai
>
> 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
> >>>>
> >>
>
>
Powered by blists - more mailing lists