Message-ID: <ee46e6d5d9a74265915345e2887338a3@sphcmbx02.sunplus.com.tw>
Date:   Fri, 26 May 2023 02:04:23 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        Dan Carpenter <dan.carpenter@...aro.org>
CC:     Wells Lu <wellslutw@...il.com>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kernel Janitors <kernel-janitors@...r.kernel.org>
Subject: RE: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

Hi,

> Le 25/05/2023 à 05:22, Wells Lu 呂芳騰 a écrit :
> >> Le 23/05/2023 à 21:37, andy.shevchenko@...il.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

Powered by blists - more mailing lists