linux-kernel - Re: [PATCH] usb: phy: isp1301: add release func to dev to avoid memory leak

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <YW2NpDG6hGJk4UR9@kroah.com>
Date:   Mon, 18 Oct 2021 17:07:16 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Yang Yingliang <yangyingliang@...wei.com>
Cc:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        balbi@...nel.org
Subject: Re: [PATCH] usb: phy: isp1301: add release func to dev to avoid
 memory leak

On Fri, Oct 15, 2021 at 01:16:24PM +0800, Yang Yingliang wrote:
> After calling usb_add_phy_dev(), client->dev.type will be changed
> to 'usb_pyh_dev_type', the release() func is null, it cause the
> following WARNING:
> 
> Device '1-001c' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
> WARNING: CPU: 1 PID: 405 at device_release+0x1b7/0x240
> Call Trace:
>  kobject_put+0x1e5/0x540
>  device_unregister+0x35/0xc0
>  i2c_unregister_device+0x114/0x1f0
> 
> It cause 'client' leaked which is allocated in i2c_new_client_device():
> 
> unreferenced object 0xffff88800670b000 (size 2048):
>   comm "xrun", pid 429, jiffies 4294946742 (age 235.248s)
>   hex dump (first 32 bytes):
>     00 00 1c 00 69 73 70 31 33 30 31 00 00 00 00 00  ....isp1301.....
>     00 00 00 00 00 00 00 00 c0 e4 17 c1 ff ff ff ff  ................
>   backtrace:
>     [<00000000a4641100>] kmem_cache_alloc_trace+0x186/0x2b0
>     [<00000000d9d933e7>] i2c_new_client_device+0x56/0xb40
>     [<000000007255bed2>] new_device_store+0x1f4/0x410
> 
> So add release func to dev to avoid this memory leak.
> 
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Fixes: 790d3a5ab6e36 ("usb: phy: isp1301: give it a context structure")
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> ---
>  drivers/usb/phy/phy-isp1301.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/phy/phy-isp1301.c b/drivers/usb/phy/phy-isp1301.c
> index ad3d57f1c273..04f005572484 100644
> --- a/drivers/usb/phy/phy-isp1301.c
> +++ b/drivers/usb/phy/phy-isp1301.c
> @@ -111,6 +111,7 @@ static int isp1301_probe(struct i2c_client *client,
>  	phy->init = isp1301_phy_init;
>  	phy->set_vbus = isp1301_phy_set_vbus;
>  	phy->type = USB_PHY_TYPE_USB2;
> +	client->dev.release = client->dev.type->release;

messing with a release pointer is almost never a good idea, and a sign
that something is really wrong.

Why is the type not set properly here so as to get the correct release
callback when the device is created?  Why do you have to manually change
this now after the fact?

thanks,

greg k-h