linux-kernel - Re: [PATCH v5 1/4] uacce: fix cdev handling in register and remove paths

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: <CABQgh9F3E7SHC3VvcbL+x6p5kJ-4=UgCMDgno=-EROcSfQT+-w@mail.gmail.com>
Date: Wed, 12 Nov 2025 19:35:46 +0800
From: Zhangfei Gao <zhangfei.gao@...aro.org>
To: Chenghai Huang <huangchenghai2@...wei.com>
Cc: gregkh@...uxfoundation.org, wangzhou1@...ilicon.com, 
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org, 
	fanghao11@...wei.com, shenyang39@...wei.com, liulongfang@...wei.com, 
	qianweili@...wei.com, linwenkai6@...ilicon.com
Subject: Re: [PATCH v5 1/4] uacce: fix cdev handling in register and remove paths

.

On Wed, 12 Nov 2025 at 17:46, Zhangfei Gao <zhangfei.gao@...aro.org> wrote:
>
> On Tue, 11 Nov 2025 at 17:35, Chenghai Huang <huangchenghai2@...wei.com> wrote:
> >
> > From: Wenkai Lin <linwenkai6@...ilicon.com>
> >
> > This patch addresses a potential issue in the uacce driver where the
> > cdev was not properly managed during error handling and cleanup paths.
> Can we clarify that it was caused by cdev_device_add?
>
> >
> > Changes made:
> > 1. In uacce_register(), store the return value of cdev_device_add()
> >    and clear the cdev owner as a flag if registration fails.
> > 2. In uacce_remove(), add additional check for cdev owner before
> >    calling cdev_device_del() to prevent potential issues.
> >
> > Fixes: 015d239ac014 ("uacce: add uacce driver")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Wenkai Lin <linwenkai6@...ilicon.com>
> > Signed-off-by: Chenghai Huang <huangchenghai2@...wei.com>
> > ---
> >  drivers/misc/uacce/uacce.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> > index 42e7d2a2a90c..688050c35d88 100644
> > --- a/drivers/misc/uacce/uacce.c
> > +++ b/drivers/misc/uacce/uacce.c
> > @@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(uacce_alloc);
> >   */
> >  int uacce_register(struct uacce_device *uacce)
> >  {
> > +       int ret;
> > +
> >         if (!uacce)
> >                 return -ENODEV;
> >
> > @@ -529,7 +531,11 @@ int uacce_register(struct uacce_device *uacce)
> >         uacce->cdev->ops = &uacce_fops;
> >         uacce->cdev->owner = THIS_MODULE;
> >
> > -       return cdev_device_add(uacce->cdev, &uacce->dev);
> > +       ret = cdev_device_add(uacce->cdev, &uacce->dev);
> > +       if (ret)
> > +               uacce->cdev->owner = NULL;
> If uacce is build in, THIS_MODULE = 0.
>
>
> how about this, handle cdev_device_add fail for no device_add case.
>
> +       if (ret) {
> +               if (!device_is_registered(&uacce->dev)) {
> +                       cdev_del(uacce->cdev);
> +                       uacce->cdev = NULL;
> +               }
> +       }
>
>
>
> > +
> > +       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(uacce_register);
> >
> > @@ -568,7 +574,7 @@ void uacce_remove(struct uacce_device *uacce)
> >                 unmap_mapping_range(q->mapping, 0, 0, 1);
> >         }
> >
> > -       if (uacce->cdev)
> > +       if (uacce->cdev && uacce->cdev->owner)
> >                 cdev_device_del(uacce->cdev, &uacce->dev);
> is there an potential issue, uacce->cdev is not freed?

And cdev_device_add(uacce->cdev, &uacce->dev) will not add refcount to
&uacce->dev
as usual when failing beforehand, so uacce_remove:
put_device(&uacce->dev) will have
refcount_t: underflow; use-after-free, need check as well.

@@ -558,8 +558,12 @@ int uacce_register(struct uacce_device *uacce)
        uacce->cdev->owner = THIS_MODULE;

        ret = cdev_device_add(uacce->cdev, &uacce->dev);
-       if (ret)
-               uacce->cdev->owner = NULL;
+       if (ret) {
+               if (!device_is_registered(&uacce->dev)) {
+                       kobject_put(&uacce->cdev->kobj);
+                       uacce->cdev = NULL;
+               }
+       }
        return ret;
 }

@@ -610,7 +614,8 @@ void uacce_remove(struct uacce_device *uacce)
        uacce->ops = NULL;
        uacce->parent = NULL;
        mutex_unlock(&uacce->mutex);
-       put_device(&uacce->dev);
+       if (device_is_registered(&uacce->dev))
+               put_device(&uacce->dev);
 }

Just one possible solution, FYI.

Thanks