diff mbox series

wifi: rtw88: Fix memory leak in rtw88_usb

Message ID 20230415011422.11162-1-Larry.Finger@lwfinger.net
State New
Headers show
Series wifi: rtw88: Fix memory leak in rtw88_usb | expand

Commit Message

Larry Finger April 15, 2023, 1:14 a.m. UTC 
Kmemleak shows the following leak arising from routine in the usb
probe routine:

unreferenced object 0xffff895cb29bba00 (size 512):
  comm "(udev-worker)", pid 534, jiffies 4294903932 (age 102751.088s)
  hex dump (first 32 bytes):
    77 30 30 30 00 00 00 00 02 2f 2d 2b 30 00 00 00  w000...../-+0...
    02 00 2a 28 00 00 00 00 ff 55 ff ff ff 00 00 00  ..*(.....U......
  backtrace:
    [<ffffffff9265fa36>] kmalloc_trace+0x26/0x90
    [<ffffffffc17eec41>] rtw_usb_probe+0x2f1/0x680 [rtw_usb]
    [<ffffffffc03e19fd>] usb_probe_interface+0xdd/0x2e0 [usbcore]
    [<ffffffff92b4f2fe>] really_probe+0x18e/0x3d0
    [<ffffffff92b4f5b8>] __driver_probe_device+0x78/0x160
    [<ffffffff92b4f6bf>] driver_probe_device+0x1f/0x90
    [<ffffffff92b4f8df>] __driver_attach+0xbf/0x1b0
    [<ffffffff92b4d350>] bus_for_each_dev+0x70/0xc0
    [<ffffffff92b4e51e>] bus_add_driver+0x10e/0x210
    [<ffffffff92b50935>] driver_register+0x55/0xf0
    [<ffffffffc03e0708>] usb_register_driver+0x88/0x140 [usbcore]
    [<ffffffff92401153>] do_one_initcall+0x43/0x210
    [<ffffffff9254f42a>] do_init_module+0x4a/0x200
    [<ffffffff92551d1c>] __do_sys_finit_module+0xac/0x120
    [<ffffffff92ee6626>] do_syscall_64+0x56/0x80
    [<ffffffff9300006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

The leak was verified to be real by unloading the driver, which resulted
in a dangling pointer to the allocation.

The allocated memory is freed in rtw_usb_disconnect().

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/usb.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Ping-Ke Shih April 17, 2023, 6:08 a.m. UTC | #1 
> -----Original Message-----
> From: Larry Finger <larry.finger@gmail.com> On Behalf Of Larry Finger
> Sent: Saturday, April 15, 2023 9:14 AM
> To: Kalle Valo <kvalo@kernel.org>
> Cc: Johannes Berg <johannes@sipsolutions.net>; linux-wireless@vger.kernel.org; Larry Finger
> <Larry.Finger@lwfinger.net>; Sascha Hauer <s.hauer@pengutronix.de>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH] wifi: rtw88: Fix memory leak in rtw88_usb
> 
> Kmemleak shows the following leak arising from routine in the usb
> probe routine:
> 
> unreferenced object 0xffff895cb29bba00 (size 512):
>   comm "(udev-worker)", pid 534, jiffies 4294903932 (age 102751.088s)
>   hex dump (first 32 bytes):
>     77 30 30 30 00 00 00 00 02 2f 2d 2b 30 00 00 00  w000...../-+0...
>     02 00 2a 28 00 00 00 00 ff 55 ff ff ff 00 00 00  ..*(.....U......
>   backtrace:
>     [<ffffffff9265fa36>] kmalloc_trace+0x26/0x90
>     [<ffffffffc17eec41>] rtw_usb_probe+0x2f1/0x680 [rtw_usb]
>     [<ffffffffc03e19fd>] usb_probe_interface+0xdd/0x2e0 [usbcore]
>     [<ffffffff92b4f2fe>] really_probe+0x18e/0x3d0
>     [<ffffffff92b4f5b8>] __driver_probe_device+0x78/0x160
>     [<ffffffff92b4f6bf>] driver_probe_device+0x1f/0x90
>     [<ffffffff92b4f8df>] __driver_attach+0xbf/0x1b0
>     [<ffffffff92b4d350>] bus_for_each_dev+0x70/0xc0
>     [<ffffffff92b4e51e>] bus_add_driver+0x10e/0x210
>     [<ffffffff92b50935>] driver_register+0x55/0xf0
>     [<ffffffffc03e0708>] usb_register_driver+0x88/0x140 [usbcore]
>     [<ffffffff92401153>] do_one_initcall+0x43/0x210
>     [<ffffffff9254f42a>] do_init_module+0x4a/0x200
>     [<ffffffff92551d1c>] __do_sys_finit_module+0xac/0x120
>     [<ffffffff92ee6626>] do_syscall_64+0x56/0x80
>     [<ffffffff9300006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> The leak was verified to be real by unloading the driver, which resulted
> in a dangling pointer to the allocation.
> 
> The allocated memory is freed in rtw_usb_disconnect().
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Ping-Ke Shih <pkshih@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 68e1b782d199..d28493a8f16c 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -882,6 +882,7 @@ void rtw_usb_disconnect(struct usb_interface *intf)
>         rtw_unregister_hw(rtwdev, hw);
>         rtw_usb_deinit_tx(rtwdev);
>         rtw_usb_deinit_rx(rtwdev);
> +       kfree(rtwusb->usb_data);

I suggest to do this in rtw_usb_intf_deinit() instead, because rtwusb->usb_data is
allocated by rtw_usb_intf_init(). Not only make the code symmetric also can handle
error cases properly of rtw_usb_probe().

Ping-Ke
Larry Finger April 17, 2023, 3:57 p.m. UTC | #2 
On 4/17/23 01:08, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Larry Finger <larry.finger@gmail.com> On Behalf Of Larry Finger
>> Sent: Saturday, April 15, 2023 9:14 AM
>> To: Kalle Valo <kvalo@kernel.org>
>> Cc: Johannes Berg <johannes@sipsolutions.net>; linux-wireless@vger.kernel.org; Larry Finger
>> <Larry.Finger@lwfinger.net>; Sascha Hauer <s.hauer@pengutronix.de>; Ping-Ke Shih <pkshih@realtek.com>
>> Subject: [PATCH] wifi: rtw88: Fix memory leak in rtw88_usb
>>
>> Kmemleak shows the following leak arising from routine in the usb
>> probe routine:
>>
>> unreferenced object 0xffff895cb29bba00 (size 512):
>>    comm "(udev-worker)", pid 534, jiffies 4294903932 (age 102751.088s)
>>    hex dump (first 32 bytes):
>>      77 30 30 30 00 00 00 00 02 2f 2d 2b 30 00 00 00  w000...../-+0...
>>      02 00 2a 28 00 00 00 00 ff 55 ff ff ff 00 00 00  ..*(.....U......
>>    backtrace:
>>      [<ffffffff9265fa36>] kmalloc_trace+0x26/0x90
>>      [<ffffffffc17eec41>] rtw_usb_probe+0x2f1/0x680 [rtw_usb]
>>      [<ffffffffc03e19fd>] usb_probe_interface+0xdd/0x2e0 [usbcore]
>>      [<ffffffff92b4f2fe>] really_probe+0x18e/0x3d0
>>      [<ffffffff92b4f5b8>] __driver_probe_device+0x78/0x160
>>      [<ffffffff92b4f6bf>] driver_probe_device+0x1f/0x90
>>      [<ffffffff92b4f8df>] __driver_attach+0xbf/0x1b0
>>      [<ffffffff92b4d350>] bus_for_each_dev+0x70/0xc0
>>      [<ffffffff92b4e51e>] bus_add_driver+0x10e/0x210
>>      [<ffffffff92b50935>] driver_register+0x55/0xf0
>>      [<ffffffffc03e0708>] usb_register_driver+0x88/0x140 [usbcore]
>>      [<ffffffff92401153>] do_one_initcall+0x43/0x210
>>      [<ffffffff9254f42a>] do_init_module+0x4a/0x200
>>      [<ffffffff92551d1c>] __do_sys_finit_module+0xac/0x120
>>      [<ffffffff92ee6626>] do_syscall_64+0x56/0x80
>>      [<ffffffff9300006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>
>> The leak was verified to be real by unloading the driver, which resulted
>> in a dangling pointer to the allocation.
>>
>> The allocated memory is freed in rtw_usb_disconnect().
>>
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Ping-Ke Shih <pkshih@realtek.com>
>> ---
>>   drivers/net/wireless/realtek/rtw88/usb.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
>> index 68e1b782d199..d28493a8f16c 100644
>> --- a/drivers/net/wireless/realtek/rtw88/usb.c
>> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
>> @@ -882,6 +882,7 @@ void rtw_usb_disconnect(struct usb_interface *intf)
>>          rtw_unregister_hw(rtwdev, hw);
>>          rtw_usb_deinit_tx(rtwdev);
>>          rtw_usb_deinit_rx(rtwdev);
>> +       kfree(rtwusb->usb_data);
> 
> I suggest to do this in rtw_usb_intf_deinit() instead, because rtwusb->usb_data is
> allocated by rtw_usb_intf_init(). Not only make the code symmetric also can handle
> error cases properly of rtw_usb_probe().

Ping-Ke,

Thanks for the suggestion. I chose my location because the allocation was just 
before the call to rtw_usb_init_tx(), thus the free went after 
rtw_usb_deinit_tx(), but I see your point. I will submit v2.

Larry
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 68e1b782d199..d28493a8f16c 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -882,6 +882,7 @@  void rtw_usb_disconnect(struct usb_interface *intf)
 	rtw_unregister_hw(rtwdev, hw);
 	rtw_usb_deinit_tx(rtwdev);
 	rtw_usb_deinit_rx(rtwdev);
+	kfree(rtwusb->usb_data);
 
 	if (rtwusb->udev->state != USB_STATE_NOTATTACHED)
 		usb_reset_device(rtwusb->udev);