Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Fri, 25 Nov 2022 19:45:21 +0100 | Subject | Re: [PATCH] powercap: fix possible name leak while device_register() fails |
| |
On Thu, Nov 24, 2022 at 3:16 AM Yang Yingliang <yangyingliang@huawei.com> wrote: > > > On 2022/11/24 3:25, Greg Kroah-Hartman wrote: > > 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@huawei.com> wrote: > >>> If device_register() returns error, the name allocated by > Sorry, > I didn't describe clearly here, it's not only after device_register() > failure, but also in the error path before register, the name is not > freed, see description below.
So you would need to update the changelog at least. But see below.
> >>> 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@huawei.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()? > powercap_register_zone() > { > ... > dev_set_name() // allocate name > ... > if (!power_zone->constraints) > goto err_const_alloc; //the name is leaked in this path > ... > if (!power_zone->zone_dev_attrs) > goto err_attr_alloc; //the name is leaked in this path > ... > if (result) > goto err_dev_ret; //the name is leaked in this path > > result = device_register(&power_zone->dev); > if (result) > goto err_dev_ret;//put_device() is not called, the name is > leaked in this path > ... > err_dev_ret: > kfree(power_zone->zone_dev_attrs); > err_attr_alloc: > kfree(power_zone->constraints); > err_const_alloc: > kfree(power_zone->name); > err_name_alloc: > idr_remove(power_zone->parent_idr, power_zone->id); > err_idr_alloc: > if (power_zone->allocated) > kfree(power_zone); > }
So can't the dev_set_name() be reordered closer to device_register(), so it is not necessary to worry about freeing the name?
> >> > >> 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. > When the dev_set_name() called in device_register(), if register fails, the > name is freed in its error path. But in this case, dev_set_name() is called > outside the register, it needs call put_device() to free the name.
In any case, device_register() needs to take care of it anyway, because it uses dev_set_name() itself in the dev->init_name case, doesn't it?
| |