| Message ID | 1684836688-9204-1-git-send-email-wellslutw@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl:sunplus: Add check for kmalloc | expand |
Tue, May 23, 2023 at 05:39:51PM +0000, Wells Lu 呂芳騰 kirjoitti: > > > Fix Smatch static checker warning: > > > potential null dereference 'configs'. (kmalloc returns null) ... > > > configs = kmalloc(sizeof(*configs), GFP_KERNEL); > > > + if (!configs) > > > > > + return -ENOMEM; > > > > "Fixing" by adding a memory leak is not probably a good approach. > > Do you mean I need to free all memory which are allocated in this subroutine before > return -ENOMEM? This is my understanding of the code. But as I said... (see below) ... > > > configs = kmalloc(sizeof(*configs), GFP_KERNEL); > > > + if (!configs) > > > + return -ENOMEM; > > > > Ditto. ... > > It might be that I'm mistaken. In this case please add an explanation why in the commit > > message. ^^^
Le 25/05/2023 à 05:22, Wells Lu 呂芳騰 a écrit : >> Le 23/05/2023 à 21:37, andy.shevchenko@gmail.com a écrit : >>> Tue, May 23, 2023 at 05:39:51PM +0000, Wells Lu 呂芳騰 kirjoitti: >>>>>> Fix Smatch static checker warning: >>>>>> potential null dereference 'configs'. (kmalloc returns null) >>> >>> ... >>> >>>>>> configs = kmalloc(sizeof(*configs), GFP_KERNEL); >>>>>> + if (!configs) >>>>> >>>>>> + return -ENOMEM; >>>>> >>>>> "Fixing" by adding a memory leak is not probably a good approach. >>>> >>>> Do you mean I need to free all memory which are allocated in this >>>> subroutine before return -ENOMEM? >>> >>> This is my understanding of the code. But as I said... (see below) >>> >>> ... >>> >>>>>> configs = kmalloc(sizeof(*configs), GFP_KERNEL); >>>>>> + if (!configs) >>>>>> + return -ENOMEM; >>>>> >>>>> Ditto. >>> >>> ... >>> >>>>> It might be that I'm mistaken. In this case please add an >>>>> explanation why in the commit message. >>> >>> ^^^ >>> >> >> Hmmm, not so sure. >> >> Should be looked at more carefully, but >> dt_to_map_one_config (in /drivers/pinctrl/devicetree.c) >> .dt_node_to_map >> --> sppctl_dt_node_to_map >> >> Should dt_to_map_one_config() fail, pinctrl_dt_free_maps() is called (see >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/devicetree.c#L281) >> >> pinctrl_dt_free_maps() calls dt_free_map(), which calls .dt_free_map, so >> pinctrl_utils_free_map() >> (see >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/sunplus/sppctl.c#L97 >> 8) >> >> Finally the needed kfree seem to be called from here. >> (see >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/pinctrl-utils.c#L119 >> ) >> >> >> *This should obviously be double checked*, but looks safe to me. >> >> >> BUT, in the same function, the of_get_parent() should be undone in case >> of error, as done at the end of the function, in the normal path. >> This one seems to be missing, should a memory allocation error occur. >> >> >> Just my 2c, >> >> CJ > > Thank you for your comments. > > From the report of kmemleak, returning -ENOMEM directly causes memory leak. We > need to free any memory allocated in this subroutine before returning -ENOMEM. > > I'll send a new patch that will free the allocated memory and call of_node_put() > when an error happens. Hi, (adding Dan in copy because the initial report is related to smatch) I don't use kmemleak, but could you share some input about its report? I've not rechecked my analysis, but it looked promising. Maybe Dan could also give a look at it and confirm your finding, or dig further with smatch to make sure that its static analysis was complete enough. CJ > > > Best regards, > Wells Lu
On Tue, May 23, 2023 at 10:05:49PM +0200, Christophe JAILLET wrote: > Le 23/05/2023 à 21:37, andy.shevchenko@gmail.com a écrit : > > Tue, May 23, 2023 at 05:39:51PM +0000, Wells Lu 呂芳騰 kirjoitti: > > > > > Fix Smatch static checker warning: > > > > > potential null dereference 'configs'. (kmalloc returns null) > > > > ... > > > > > > > configs = kmalloc(sizeof(*configs), GFP_KERNEL); > > > > > + if (!configs) > > > > > > > > > + return -ENOMEM; > > > > > > > > "Fixing" by adding a memory leak is not probably a good approach. > > > > > > Do you mean I need to free all memory which are allocated in this subroutine before > > > return -ENOMEM? > > > > This is my understanding of the code. But as I said... (see below) > > > > ... > > > > > > > configs = kmalloc(sizeof(*configs), GFP_KERNEL); > > > > > + if (!configs) > > > > > + return -ENOMEM; > > > > > > > > Ditto. > > > > ... > > > > > > It might be that I'm mistaken. In this case please add an explanation why in the commit > > > > message. > > > > ^^^ > > > > Hmmm, not so sure. > > Should be looked at more carefully, but > dt_to_map_one_config (in /drivers/pinctrl/devicetree.c) > .dt_node_to_map > --> sppctl_dt_node_to_map > > Should dt_to_map_one_config() fail, pinctrl_dt_free_maps() is called > (see https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/devicetree.c#L281) Thanks for this call tree, I don't have this file enabled in my build so it's not easy for me to find how sppctl_dt_node_to_map() was called. drivers/pinctrl/devicetree.c 160 dev_err(p->dev, "pctldev %s doesn't support DT\n", 161 dev_name(pctldev->dev)); 162 return -ENODEV; 163 } 164 ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps); ^^^^ "map" isn't stored anywhere so it will be leaked. I guess kmemleak already figured this out. 165 if (ret < 0) 166 return ret; 167 else if (num_maps == 0) { 168 /* > > pinctrl_dt_free_maps() calls dt_free_map(), which calls .dt_free_map, so > pinctrl_utils_free_map() > (see https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/sunplus/sppctl.c#L978) > > Finally the needed kfree seem to be called from here. > (see https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/pinctrl-utils.c#L119) > > > *This should obviously be double checked*, but looks safe to me. > > > BUT, in the same function, the of_get_parent() should be undone in case of > error, as done at the end of the function, in the normal path. > This one seems to be missing, should a memory allocation error occur. > Yes. There are some missing calls to of_node_put(parent); in sppctl_dt_node_to_map(). regards, dan carpenter
On Thu, May 25, 2023 at 08:37:36PM +0200, Christophe JAILLET wrote: > > Hi, > (adding Dan in copy because the initial report is related to smatch) > > I don't use kmemleak, but could you share some input about its report? > > > I've not rechecked my analysis, but it looked promising. > Maybe Dan could also give a look at it and confirm your finding, or dig > further with smatch to make sure that its static analysis was complete > enough. Smatch doesn't really complain about memory leaks. I wrote that check 13 years ago and focused more on avoiding false positives instead of being thorough. It turns out that avoiding false positives is achievable but pretty useless. Checking for the of_get_parent() leaks is probably easier. I could just add it to check_unwind.c or create something custom. The heuristic for the custom check would be to print a warning if the success path releases it but the others don't. regards, dan carpenter
Le 25/05/2023 à 21:19, Dan Carpenter a écrit : > On Tue, May 23, 2023 at 10:05:49PM +0200, Christophe JAILLET wrote: >> Should be looked at more carefully, but >> dt_to_map_one_config (in /drivers/pinctrl/devicetree.c) >> .dt_node_to_map >> --> sppctl_dt_node_to_map >> >> Should dt_to_map_one_config() fail, pinctrl_dt_free_maps() is called >> (see https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/devicetree.c#L281) > > Thanks for this call tree, I don't have this file enabled in my build > so it's not easy for me to find how sppctl_dt_node_to_map() was called. > > drivers/pinctrl/devicetree.c > 160 dev_err(p->dev, "pctldev %s doesn't support DT\n", > 161 dev_name(pctldev->dev)); > 162 return -ENODEV; > 163 } > 164 ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps); > ^^^^ > "map" isn't stored anywhere so it will be leaked. I guess kmemleak > already figured this out. > > 165 if (ret < 0) > 166 return ret; > 167 else if (num_maps == 0) { > 168 /* > Hi, thanks Dan for sharing your PoV on this. CJ
Hi, > Le 25/05/2023 à 05:22, Wells Lu 呂芳騰 a écrit : > >> Le 23/05/2023 à 21:37, andy.shevchenko@gmail.com a écrit : > >>> Tue, May 23, 2023 at 05:39:51PM +0000, Wells Lu 呂芳騰 kirjoitti: > >>>>>> Fix Smatch static checker warning: > >>>>>> potential null dereference 'configs'. (kmalloc returns null) > >>> > >>> ... > >>> > >>>>>> configs = kmalloc(sizeof(*configs), GFP_KERNEL); > >>>>>> + if (!configs) > >>>>> > >>>>>> + return -ENOMEM; > >>>>> > >>>>> "Fixing" by adding a memory leak is not probably a good approach. > >>>> > >>>> Do you mean I need to free all memory which are allocated in this > >>>> subroutine before return -ENOMEM? > >>> > >>> This is my understanding of the code. But as I said... (see below) > >>> > >>> ... > >>> > >>>>>> configs = kmalloc(sizeof(*configs), GFP_KERNEL); > >>>>>> + if (!configs) > >>>>>> + return -ENOMEM; > >>>>> > >>>>> Ditto. > >>> > >>> ... > >>> > >>>>> It might be that I'm mistaken. In this case please add an > >>>>> explanation why in the commit message. > >>> > >>> ^^^ > >>> > >> > >> Hmmm, not so sure. > >> > >> Should be looked at more carefully, but > >> dt_to_map_one_config (in /drivers/pinctrl/devicetree.c) > >> .dt_node_to_map > >> --> sppctl_dt_node_to_map > >> > >> Should dt_to_map_one_config() fail, pinctrl_dt_free_maps() is called > >> (see > >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/devi > >> cetree.c#L281) > >> > >> pinctrl_dt_free_maps() calls dt_free_map(), which calls .dt_free_map, > >> so > >> pinctrl_utils_free_map() > >> (see > >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/sunp > >> lus/sppctl.c#L97 > >> 8) > >> > >> Finally the needed kfree seem to be called from here. > >> (see > >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/pinc > >> trl-utils.c#L119 > >> ) > >> > >> > >> *This should obviously be double checked*, but looks safe to me. > >> > >> > >> BUT, in the same function, the of_get_parent() should be undone in > >> case of error, as done at the end of the function, in the normal path. > >> This one seems to be missing, should a memory allocation error occur. > >> > >> > >> Just my 2c, > >> > >> CJ > > > > Thank you for your comments. > > > > From the report of kmemleak, returning -ENOMEM directly causes memory > > leak. We need to free any memory allocated in this subroutine before returning -ENOMEM. > > > > I'll send a new patch that will free the allocated memory and call > > of_node_put() when an error happens. > > Hi, > (adding Dan in copy because the initial report is related to smatch) > > I don't use kmemleak, but could you share some input about its report? > > > I've not rechecked my analysis, but it looked promising. > Maybe Dan could also give a look at it and confirm your finding, or dig further with smatch > to make sure that its static analysis was complete enough. > > CJ I forced sppctl_dt_node_to_map() to always return -ENOMEM when it configs GPIO pins. Here is the report of kmemleak: ~ # echo scan > /sys/kernel/debug/kmemleak [ 9.136464] kmemleak: 2 new suspected memory leaks (see /sys/kernel/debug/kmemleak) ~ # ~ # cat /sys/kernel/debug/kmemleak unreferenced object 0xc2852e00 (size 64): comm "kworker/u8:3", pid 36, jiffies 4294937312 (age 13.610s) hex dump (first 32 bytes): 00 00 00 00 14 7c ff df 03 00 00 00 00 00 00 00 .....|.......... c4 8f 3a c0 00 00 00 00 01 00 00 00 00 00 00 00 ..:............. backtrace: [<(ptrval)>] slab_post_alloc_hook.constprop.16+0x4b/0x52 [<(ptrval)>] __kmem_cache_alloc_node+0x57/0x78 [<(ptrval)>] __kmalloc+0x33/0x3a [<(ptrval)>] sppctl_dt_node_to_map+0x77/0x3cc [<(ptrval)>] pinctrl_dt_to_map+0x16f/0x20e [<(ptrval)>] create_pinctrl+0x3d/0x224 [<(ptrval)>] devm_pinctrl_get+0x23/0x50 [<(ptrval)>] pinctrl_bind_pins+0x31/0x190 [<(ptrval)>] really_probe+0x89/0x23e [<(ptrval)>] __driver_probe_device+0x131/0x164 [<(ptrval)>] driver_probe_device+0x2d/0x88 [<(ptrval)>] __device_attach_driver+0x8d/0xa4 [<(ptrval)>] bus_for_each_drv+0x63/0x72 [<(ptrval)>] __device_attach+0x89/0xe2 [<(ptrval)>] bus_probe_device+0x1f/0x5a [<(ptrval)>] deferred_probe_work_func+0x69/0x88 unreferenced object 0xc2852dc0 (size 64): comm "kworker/u8:3", pid 36, jiffies 4294937312 (age 13.610s) hex dump (first 32 bytes): 01 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 54 44 00 00 00 00 00 02 ........TD...... backtrace: [<(ptrval)>] slab_post_alloc_hook.constprop.16+0x4b/0x52 [<(ptrval)>] __kmem_cache_alloc_node+0x57/0x78 [<(ptrval)>] kmalloc_trace+0x11/0x16 [<(ptrval)>] sppctl_dt_node_to_map+0x14b/0x3cc [<(ptrval)>] pinctrl_dt_to_map+0x16f/0x20e [<(ptrval)>] create_pinctrl+0x3d/0x224 [<(ptrval)>] devm_pinctrl_get+0x23/0x50 [<(ptrval)>] pinctrl_bind_pins+0x31/0x190 [<(ptrval)>] really_probe+0x89/0x23e [<(ptrval)>] __driver_probe_device+0x131/0x164 [<(ptrval)>] driver_probe_device+0x2d/0x88 [<(ptrval)>] __device_attach_driver+0x8d/0xa4 [<(ptrval)>] bus_for_each_drv+0x63/0x72 [<(ptrval)>] __device_attach+0x89/0xe2 [<(ptrval)>] bus_probe_device+0x1f/0x5a [<(ptrval)>] deferred_probe_work_func+0x69/0x88 ~ # Best regards, Wells Lu
diff --git a/drivers/pinctrl/sunplus/sppctl.c b/drivers/pinctrl/sunplus/sppctl.c index 6bbbab3a6..f2dcc68ee 100644 --- a/drivers/pinctrl/sunplus/sppctl.c +++ b/drivers/pinctrl/sunplus/sppctl.c @@ -883,6 +883,8 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node (*map)[i].data.configs.num_configs = 1; (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num); configs = kmalloc(sizeof(*configs), GFP_KERNEL); + if (!configs) + return -ENOMEM; *configs = FIELD_GET(GENMASK(7, 0), dt_pin); (*map)[i].data.configs.configs = configs; @@ -896,6 +898,8 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node (*map)[i].data.configs.num_configs = 1; (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num); configs = kmalloc(sizeof(*configs), GFP_KERNEL); + if (!configs) + return -ENOMEM; *configs = SPPCTL_IOP_CONFIGS; (*map)[i].data.configs.configs = configs;
Fix Smatch static checker warning: potential null dereference 'configs'. (kmalloc returns null) Fixes: aa74c44be19c ("pinctrl: Add driver for Sunplus SP7021") Signed-off-by: Wells Lu <wellslutw@gmail.com> --- drivers/pinctrl/sunplus/sppctl.c | 4 ++++ 1 file changed, 4 insertions(+)