linux-kernel - Re: [PATCH] powercap: fix possible name leak while device

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 23 Nov 2022 20:25:53 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Yang Yingliang <yangyingliang@...wei.com>,
        linux-pm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] powercap: fix possible name leak while device_register()
 fails

On Wed, Nov 23, 2022 at 08:00:14PM +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang
> <yangyingliang@...wei.com> wrote:
> >
> > If device_register() returns error, the name allocated by
> > dev_set_name() need be freed. In technical, we should call
> > put_device() to give up the reference and free the name in
> > driver core, but in some cases the device is not intizalized,
> > put_device() can not be called, so don't complicate the code,
> > just call kfree_const() to free name in the error path.
> >
> > Fixes: 75d2364ea0ca ("PowerCap: Add class driver")
> > Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> > ---
> >  drivers/powercap/powercap_sys.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
> > index f0654a932b37..11e742dc83b9 100644
> > --- a/drivers/powercap/powercap_sys.c
> > +++ b/drivers/powercap/powercap_sys.c
> > @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone(
> >  err_name_alloc:
> >         idr_remove(power_zone->parent_idr, power_zone->id);
> >  err_idr_alloc:
> > +       kfree_const(dev_name(&power_zone->dev));
> >         if (power_zone->allocated)
> >                 kfree(power_zone);
> >         mutex_unlock(&control_type->lock);
> > @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type(
> >         dev_set_name(&control_type->dev, "%s", name);
> >         result = device_register(&control_type->dev);
> >         if (result) {
> > +               kfree_const(dev_name(&control_type->dev));
> 
> Why is it necessary to free a device name explicitly after a failing
> device_register()?
> 
> If it is really necessary, then there is a problem in
> device_register() itself AFAICS, because it uses dev_set_name() at
> least in the dev->init_name present case.

I think we already fixed this in the driver core, so these types of
patches should not be applied.

Yang, can you make sure you respond to all of them and say "this is not
needed anymore!" and if any got merged, send reverts for them?

thanks,

greg k-h