From: Quanyang Wang <[email protected]>
In gpiochip_add_data_with_key, we should check the return value of
dev_set_name to ensure that device name is allocated successfully
and then add a label on the error path to free device name to fix
kmemleak as below:
unreferenced object 0xc2d6fc40 (size 64):
comm "kworker/0:1", pid 16, jiffies 4294937425 (age 65.120s)
hex dump (first 32 bytes):
67 70 69 6f 63 68 69 70 30 00 1a c0 54 63 1a c0 gpiochip0...Tc..
0c ed 84 c0 48 ed 84 c0 3c ee 84 c0 10 00 00 00 ....H...<.......
backtrace:
[<962810f7>] kobject_set_name_vargs+0x2c/0xa0
[<f50797e6>] dev_set_name+0x2c/0x5c
[<94abbca9>] gpiochip_add_data_with_key+0xfc/0xce8
[<5c4193e0>] omap_gpio_probe+0x33c/0x68c
[<3402f137>] platform_probe+0x58/0xb8
[<7421e210>] really_probe+0xec/0x3b4
[<000f8ada>] driver_probe_device+0x58/0xb4
[<67e0f7f7>] bus_for_each_drv+0x80/0xd0
[<4de545dc>] __device_attach+0xe8/0x15c
[<2e4431e7>] bus_probe_device+0x84/0x8c
[<c18b1de9>] device_add+0x384/0x7c0
[<5aff2995>] of_platform_device_create_pdata+0x8c/0xb8
[<061c3483>] of_platform_bus_create+0x198/0x230
[<5ee6d42a>] of_platform_populate+0x60/0xb8
[<2647300f>] sysc_probe+0xd18/0x135c
[<3402f137>] platform_probe+0x58/0xb8
Signed-off-by: Quanyang Wang <[email protected]>
---
drivers/gpio/gpiolib.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7e1ad4d40e0a..091e00f2e0a9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -603,7 +603,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
ret = gdev->id;
goto err_free_gdev;
}
- dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
+
+ ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
+ if (ret)
+ goto err_free_ida;
+
device_initialize(&gdev->dev);
dev_set_drvdata(&gdev->dev, gdev);
if (gc->parent && gc->parent->driver)
@@ -617,7 +621,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
gdev->descs = kcalloc(gc->ngpio, sizeof(gdev->descs[0]), GFP_KERNEL);
if (!gdev->descs) {
ret = -ENOMEM;
- goto err_free_ida;
+ goto err_free_dev_name;
}
if (gc->ngpio == 0) {
@@ -768,6 +772,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
kfree_const(gdev->label);
err_free_descs:
kfree(gdev->descs);
+err_free_dev_name:
+ kfree(dev_name(&gdev->dev));
err_free_ida:
ida_free(&gpio_ida, gdev->id);
err_free_gdev:
--
2.25.1
On Fri, Jan 29, 2021 at 2:01 PM <[email protected]> wrote:
>
> From: Quanyang Wang <[email protected]>
>
> In gpiochip_add_data_with_key, we should check the return value of
> dev_set_name to ensure that device name is allocated successfully
> and then add a label on the error path to free device name to fix
> kmemleak as below:
Thanks for the report.
Unfortunately...
> + ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
> + if (ret)
> + goto err_free_ida;
...
> +err_free_dev_name:
> + kfree(dev_name(&gdev->dev));
...this approach seems to create a possible double free if I'm not mistaken.
The idea is that device name should be cleaned in kobject ->release()
callback when device is put.
Can you elaborate?
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 1/30/21 1:26 AM, Andy Shevchenko wrote:
> On Fri, Jan 29, 2021 at 2:01 PM <[email protected]> wrote:
>> From: Quanyang Wang <[email protected]>
>>
>> In gpiochip_add_data_with_key, we should check the return value of
>> dev_set_name to ensure that device name is allocated successfully
>> and then add a label on the error path to free device name to fix
>> kmemleak as below:
> Thanks for the report.
> Unfortunately...
>
>> + ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
>> + if (ret)
>> + goto err_free_ida;
> ...
>
>> +err_free_dev_name:
>> + kfree(dev_name(&gdev->dev));
> ...this approach seems to create a possible double free if I'm not mistaken.
Thanks for your comment. I didn't catch the double free. Would you
please point it out?
>
> The idea is that device name should be cleaned in kobject ->release()
> callback when device is put.
Yes, the device name should be freed by calling put_device(&gdev->dev).
But int gpiochip_add_data_with_key,
when running dev_set_name, "gdev->dev.release" hasn't been installed
until in the tail of gpiochip_add_data_with_key.
So we couldn't call put_device here.
Any suggestion is much appreciated.
Thanks,
Quanyang
> Can you elaborate?
>
On Sat, Jan 30, 2021 at 4:45 AM quanyang.wang
<[email protected]> wrote:
>
> Hi Andy,
>
> On 1/30/21 1:26 AM, Andy Shevchenko wrote:
> > On Fri, Jan 29, 2021 at 2:01 PM <[email protected]> wrote:
> >> From: Quanyang Wang <[email protected]>
> >>
> >> In gpiochip_add_data_with_key, we should check the return value of
> >> dev_set_name to ensure that device name is allocated successfully
> >> and then add a label on the error path to free device name to fix
> >> kmemleak as below:
> > Thanks for the report.
> > Unfortunately...
> >
> >> + ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
> >> + if (ret)
> >> + goto err_free_ida;
> > ...
> >
> >> +err_free_dev_name:
> >> + kfree(dev_name(&gdev->dev));
> > ...this approach seems to create a possible double free if I'm not mistaken.
> Thanks for your comment. I didn't catch the double free. Would you
> please point it out?
> >
> > The idea is that device name should be cleaned in kobject ->release()
> > callback when device is put.
>
> Yes, the device name should be freed by calling put_device(&gdev->dev).
> But int gpiochip_add_data_with_key,
>
> when running dev_set_name, "gdev->dev.release" hasn't been installed
> until in the tail of gpiochip_add_data_with_key.
>
> So we couldn't call put_device here.
>
> Any suggestion is much appreciated.
>
> Thanks,
>
> Quanyang
>
> > Can you elaborate?
> >
Andy,
gdev->dev.release is assigned as the very last step in
gpiochip_add_data_with_key() so the patch looks correct to me. Do you
still have objections? Maybe I'm not seeing something.
Bart
On Mon, Feb 1, 2021 at 5:50 PM Bartosz Golaszewski
<[email protected]> wrote:
>
> On Sat, Jan 30, 2021 at 4:45 AM quanyang.wang
> <[email protected]> wrote:
> >
> > Hi Andy,
> >
> > On 1/30/21 1:26 AM, Andy Shevchenko wrote:
> > > On Fri, Jan 29, 2021 at 2:01 PM <[email protected]> wrote:
> > >> From: Quanyang Wang <[email protected]>
> > >>
> > >> In gpiochip_add_data_with_key, we should check the return value of
> > >> dev_set_name to ensure that device name is allocated successfully
> > >> and then add a label on the error path to free device name to fix
> > >> kmemleak as below:
> > > Thanks for the report.
> > > Unfortunately...
> > >
> > >> + ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
> > >> + if (ret)
> > >> + goto err_free_ida;
> > > ...
> > >
> > >> +err_free_dev_name:
> > >> + kfree(dev_name(&gdev->dev));
> > > ...this approach seems to create a possible double free if I'm not mistaken.
> > Thanks for your comment. I didn't catch the double free. Would you
> > please point it out?
> > >
> > > The idea is that device name should be cleaned in kobject ->release()
> > > callback when device is put.
> >
> > Yes, the device name should be freed by calling put_device(&gdev->dev).
> > But int gpiochip_add_data_with_key,
> >
> > when running dev_set_name, "gdev->dev.release" hasn't been installed
> > until in the tail of gpiochip_add_data_with_key.
> >
> > So we couldn't call put_device here.
> >
> > Any suggestion is much appreciated.
> >
> > Thanks,
> >
> > Quanyang
> >
> > > Can you elaborate?
> > >
>
> Andy,
>
> gdev->dev.release is assigned as the very last step in
> gpiochip_add_data_with_key() so the patch looks correct to me. Do you
> still have objections? Maybe I'm not seeing something.
OK! (Sorry, don't have time to look deeper, just remember that netdev
code used to (or still?) have some twisted cases with device
registration and similar syzcaller issue, but in that case it wasn't
so easy to fix.
--
With Best Regards,
Andy Shevchenko