Skip to content

Commit 89988b4

Browse files
committed
usbnet: Fix use-after-free on disconnect
usbnet uses the work usbnet_deferred_kevent() to perform tasks which may sleep. On disconnect, completion of the work was originally awaited in ->ndo_stop(). But in 2003, that was moved to ->disconnect() by historic commit "[PATCH] USB: usbnet, prevent exotic rtnl deadlock": https://git.kernel.org/tglx/history/c/0f138bbfd83c The change was necessary because back then, the kernel's workqueue implementation did not allow waiting for a single work. One had to wait for completion of *all* work by calling flush_scheduled_work(), and that could deadlock when waiting for usbnet_deferred_kevent() with rtnl_mutex held in ->ndo_stop(). The commit solved one problem but created another: It introduced the possibility of a use-after-free in USB Ethernet drivers aqc111.c, asix_devices.c, ax88179_178a.c, ch9200.c and smsc75xx.c: * If the drivers receive a link change interrupt immediately before disconnect, they raise EVENT_LINK_RESET in their (non-sleepable) ->status() callback and schedule usbnet_deferred_kevent(). * usbnet_deferred_kevent() invokes the driver's ->link_reset() callback, which calls netif_carrier_{on,off}(). * That in turn schedules the work linkwatch_event(). Because usbnet_deferred_kevent() is awaited after unregister_netdev(), netif_carrier_{on,off}() may operate on an unregistered net_device and linkwatch_event() may run after free_netdev(), causing a use-after-free. In 2010, usbnet was changed to only wait for a single instance of usbnet_deferred_kevent() instead of *all* work by commit 23f333a ("drivers/net: don't use flush_scheduled_work()"). Unfortunately the commit neglected to move the wait back to ->ndo_stop(). Rectify that omission at long last. Reported-by: Jann Horn <jannh@google.com> Link: https://lore.kernel.org/netdev/CAG48ez0MHBbENX5gCdHAUXZ7h7s20LnepBF-pa5M=7Bi-jZrEA@mail.gmail.com/ Reported-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/netdev/20220315113841.GA22337@pengutronix.de/ Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org Cc: Oliver Neukum <oneukum@suse.com>
1 parent dcb6823 commit 89988b4

File tree

1 file changed

+2
-6
lines changed

1 file changed

+2
-6
lines changed

drivers/net/usb/usbnet.c

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -849,13 +849,11 @@ int usbnet_stop (struct net_device *net)
849849

850850
mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
851851

852-
/* deferred work (task, timer, softirq) must also stop.
853-
* can't flush_scheduled_work() until we drop rtnl (later),
854-
* else workers could deadlock; so make workers a NOP.
855-
*/
852+
/* deferred work (task, timer, softirq) must also stop. */
856853
dev->flags = 0;
857854
del_timer_sync (&dev->delay);
858855
tasklet_kill (&dev->bh);
856+
cancel_work_sync(&dev->kevent);
859857
if (!pm)
860858
usb_autopm_put_interface(dev->intf);
861859

@@ -1619,8 +1617,6 @@ void usbnet_disconnect (struct usb_interface *intf)
16191617
net = dev->net;
16201618
unregister_netdev (net);
16211619

1622-
cancel_work_sync(&dev->kevent);
1623-
16241620
usb_scuttle_anchored_urbs(&dev->deferred);
16251621

16261622
if (dev->driver_info->unbind)

0 commit comments

Comments
 (0)