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] [day] [month] [year] [list]
Message-ID: <CAAhV-H7iETpPUqVYoOXDNX53BMx1AQAtDD12VXw8GD=H1YSZpQ@mail.gmail.com>
Date: Wed, 3 Sep 2025 16:34:49 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Bibo Mao <maobibo@...ngson.cn>
Cc: Xianglai Li <lixianglai@...ngson.cn>, WANG Xuerui <kernel@...0n.name>, kvm@...r.kernel.org, 
	loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock
 hold in kvm_eiointc_regs_access

On Tue, Sep 2, 2025 at 8:17 PM Bibo Mao <maobibo@...ngson.cn> wrote:
>
>
>
> On 2025/9/2 下午7:58, Huacai Chen wrote:
> > Hi, Bibo,
> >
> > On Tue, Sep 2, 2025 at 5:49 PM Bibo Mao <maobibo@...ngson.cn> wrote:
> >>
> >> Function copy_from_user() and copy_to_user() may sleep because of page
> >> fault, and they cannot be called in spin_lock hold context. Otherwise there
> >> will be possible warning such as:
> >>
> >> BUG: sleeping function called from invalid context at include/linux/uaccess.h:192
> >> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 6292, name: qemu-system-loo
> >> preempt_count: 1, expected: 0
> >> RCU nest depth: 0, expected: 0
> >> INFO: lockdep is turned off.
> >> irq event stamp: 0
> >> hardirqs last  enabled at (0): [<0000000000000000>] 0x0
> >> hardirqs last disabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
> >> softirqs last  enabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
> >> softirqs last disabled at (0): [<0000000000000000>] 0x0
> >> CPU: 41 UID: 0 PID: 6292 Comm: qemu-system-loo Tainted: G W 6.17.0-rc3+ #31 PREEMPT(full)
> >> Tainted: [W]=WARN
> >> Stack : 0000000000000076 0000000000000000 9000000004c28264 9000100092ff4000
> >>          9000100092ff7b80 9000100092ff7b88 0000000000000000 9000100092ff7cc8
> >>          9000100092ff7cc0 9000100092ff7cc0 9000100092ff7a00 0000000000000001
> >>          0000000000000001 9000100092ff7b88 947d2f9216a5e8b9 900010008773d880
> >>          00000000ffff8b9f fffffffffffffffe 0000000000000ba1 fffffffffffffffe
> >>          000000000000003e 900000000825a15b 000010007ad38000 9000100092ff7ec0
> >>          0000000000000000 0000000000000000 9000000006f3ac60 9000000007252000
> >>          0000000000000000 00007ff746ff2230 0000000000000053 9000200088a021b0
> >>          0000555556c9d190 0000000000000000 9000000004c2827c 000055556cfb5f40
> >>          00000000000000b0 0000000000000007 0000000000000007 0000000000071c1d
> >> Call Trace:
> >> [<9000000004c2827c>] show_stack+0x5c/0x180
> >> [<9000000004c20fac>] dump_stack_lvl+0x94/0xe4
> >> [<9000000004c99c7c>] __might_resched+0x26c/0x290
> >> [<9000000004f68968>] __might_fault+0x20/0x88
> >> [<ffff800002311de0>] kvm_eiointc_regs_access.isra.0+0x88/0x380 [kvm]
> >> [<ffff8000022f8514>] kvm_device_ioctl+0x194/0x290 [kvm]
> >> [<900000000506b0d8>] sys_ioctl+0x388/0x1010
> >> [<90000000063ed210>] do_syscall+0xb0/0x2d8
> >> [<9000000004c25ef8>] handle_syscall+0xb8/0x158
> >>
> >> Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
> >> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
> >> ---
> >>   arch/loongarch/kvm/intc/eiointc.c | 33 ++++++++++++++++++++-----------
> >>   1 file changed, 21 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> >> index 026b139dcff2..2fb5b9c6e8ad 100644
> >> --- a/arch/loongarch/kvm/intc/eiointc.c
> >> +++ b/arch/loongarch/kvm/intc/eiointc.c
> >> @@ -462,19 +462,17 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev,
> >>
> >>   static int kvm_eiointc_regs_access(struct kvm_device *dev,
> >>                                          struct kvm_device_attr *attr,
> >> -                                       bool is_write)
> >> +                                       bool is_write, int *data)
> >>   {
> >>          int addr, cpu, offset, ret = 0;
> >>          unsigned long flags;
> >>          void *p = NULL;
> >> -       void __user *data;
> >>          struct loongarch_eiointc *s;
> >>
> >>          s = dev->kvm->arch.eiointc;
> >>          addr = attr->attr;
> >>          cpu = addr >> 16;
> >>          addr &= 0xffff;
> >> -       data = (void __user *)attr->addr;
> >>          switch (addr) {
> >>          case EIOINTC_NODETYPE_START ... EIOINTC_NODETYPE_END:
> >>                  offset = (addr - EIOINTC_NODETYPE_START) / 4;
> >> @@ -513,13 +511,10 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,
> >>          }
> >>
> >>          spin_lock_irqsave(&s->lock, flags);
> >> -       if (is_write) {
> >> -               if (copy_from_user(p, data, 4))
> >> -                       ret = -EFAULT;
> >> -       } else {
> >> -               if (copy_to_user(data, p, 4))
> >> -                       ret = -EFAULT;
> >> -       }
> >> +       if (is_write)
> >> +               memcpy(p, data, 4);
> >> +       else
> >> +               memcpy(data, p, 4);
> > p is a local variable, data is a parameter, they both have nothing to
> > do with s, why memcpy need to be protected?
> p is pointer to register buffer rather than local variable. When dump
> extioi register to user space, maybe one vCPU is writing extioi register
> at the same time, so there needs spinlock protection.
Make sense, applied.

Huacai

>
> >
> > After some thinking I found the code was wrong at the first time.  The
> > real code that needs to be protected is not copy_from_user() or
> > memcpy(), but the above switch block.
> For switch block in function kvm_eiointc_regs_access() for example, it
> is only to get register buffer pointer, not register content. Spinlock
> protection is not necessary in switch block.
>
> Regards
> Bibo Mao
> >
> > Other patches have similar problems.
> >
> > Huacai
> >
> >>          spin_unlock_irqrestore(&s->lock, flags);
> >>
> >>          return ret;
> >> @@ -576,9 +571,18 @@ static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
> >>   static int kvm_eiointc_get_attr(struct kvm_device *dev,
> >>                                  struct kvm_device_attr *attr)
> >>   {
> >> +       int ret, data;
> >> +
> >>          switch (attr->group) {
> >>          case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
> >> -               return kvm_eiointc_regs_access(dev, attr, false);
> >> +               ret = kvm_eiointc_regs_access(dev, attr, false, &data);
> >> +               if (ret)
> >> +                       return ret;
> >> +
> >> +               if (copy_to_user((void __user *)attr->addr, &data, 4))
> >> +                       ret = -EFAULT;
> >> +
> >> +               return ret;
> >>          case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
> >>                  return kvm_eiointc_sw_status_access(dev, attr, false);
> >>          default:
> >> @@ -589,11 +593,16 @@ static int kvm_eiointc_get_attr(struct kvm_device *dev,
> >>   static int kvm_eiointc_set_attr(struct kvm_device *dev,
> >>                                  struct kvm_device_attr *attr)
> >>   {
> >> +       int data;
> >> +
> >>          switch (attr->group) {
> >>          case KVM_DEV_LOONGARCH_EXTIOI_GRP_CTRL:
> >>                  return kvm_eiointc_ctrl_access(dev, attr);
> >>          case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
> >> -               return kvm_eiointc_regs_access(dev, attr, true);
> >> +               if (copy_from_user(&data, (void __user *)attr->addr, 4))
> >> +                       return -EFAULT;
> >> +
> >> +               return kvm_eiointc_regs_access(dev, attr, true, &data);
> >>          case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
> >>                  return kvm_eiointc_sw_status_access(dev, attr, true);
> >>          default:
> >> --
> >> 2.39.3
> >>
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ