| Message ID | d1c87ebe9fc502bffcd1576e238d685ad08321e4.1655987888.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a69e617e533edddf3fa3123149900f36e0a6dc74 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] usbnet: Fix linkwatch use-after-free on disconnect | expand |
On 23.06.22 14:50, Lukas Wunner wrote: > 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 > Cc: Oliver Neukum <oneukum@suse.com> Acked-by: Oliver Neukum <oneukum@suse.com>
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Thu, 23 Jun 2022 14:50:59 +0200 you wrote: > 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 > > [...] Here is the summary with links: - [net-next,v2] usbnet: Fix linkwatch use-after-free on disconnect https://git.kernel.org/netdev/net-next/c/a69e617e533e You are awesome, thank you!
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index dc79811535c2..63868c1fbea4 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)
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 Cc: Oliver Neukum <oneukum@suse.com> --- This seems to be Jakub's preferred way to solve the use-after-free. I've tagged the patch for net-next so that it can bake in linux-next for a few weeks, but it alternatively applies cleanly to net (in case maintainers are eager to get it into mainline). The patch supersedes the following prior attempts: usbnet: Fix use-after-free on disconnect https://lore.kernel.org/netdev/127121d9d933ebe3fc13f9f91cc33363d6a8a8ac.1649859147.git.lukas@wunner.de/ net: linkwatch: ignore events for unregistered netdevs https://lore.kernel.org/netdev/18b3541e5372bc9b9fc733d422f4e698c089077c.1650177997.git.lukas@wunner.de/ net: linkwatch: ignore events for unregistered netdevs (v2) https://lore.kernel.org/netdev/cover.1655024266.git.lukas@wunner.de/ drivers/net/usb/usbnet.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)