| Message ID | 20230415011422.11162-1-Larry.Finger@lwfinger.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: rtw88: Fix memory leak in rtw88_usb | expand |
> -----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
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 --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);
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(+)