diff options
author | Lukas Wunner <lukas@wunner.de> | 2022-06-23 14:50:59 +0200 |
---|---|---|
committer | Jakub Kicinski <kuba@kernel.org> | 2022-06-24 16:44:42 -0700 |
commit | a69e617e533edddf3fa3123149900f36e0a6dc74 (patch) | |
tree | 60d69bddb6da0b313dfa1955bfe74e2c155685d1 /drivers/net/usb | |
parent | 78f319315764e6730c143570609b230d1bea6928 (diff) | |
download | linux-a69e617e533edddf3fa3123149900f36e0a6dc74.tar.gz linux-a69e617e533edddf3fa3123149900f36e0a6dc74.tar.bz2 linux-a69e617e533edddf3fa3123149900f36e0a6dc74.zip |
usbnet: Fix linkwatch 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 made 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 causes 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 netdev 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 23f333a2bfaf
("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
Acked-by: Oliver Neukum <oneukum@suse.com>
Link: https://lore.kernel.org/r/d1c87ebe9fc502bffcd1576e238d685ad08321e4.1655987888.git.lukas@wunner.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Diffstat (limited to 'drivers/net/usb')
-rw-r--r-- | drivers/net/usb/usbnet.c | 8 |
1 files changed, 2 insertions, 6 deletions
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index dc79811535c2..713cfc2723b8 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -849,13 +849,11 @@ int usbnet_stop (struct net_device *net) mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags); - /* deferred work (task, timer, softirq) must also stop. - * can't flush_scheduled_work() until we drop rtnl (later), - * else workers could deadlock; so make workers a NOP. - */ + /* deferred work (timer, softirq, task) must also stop */ dev->flags = 0; del_timer_sync (&dev->delay); tasklet_kill (&dev->bh); + cancel_work_sync(&dev->kevent); if (!pm) usb_autopm_put_interface(dev->intf); @@ -1619,8 +1617,6 @@ void usbnet_disconnect (struct usb_interface *intf) net = dev->net; unregister_netdev (net); - cancel_work_sync(&dev->kevent); - usb_scuttle_anchored_urbs(&dev->deferred); if (dev->driver_info->unbind) |