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@xxxxxxxxxx> > Link: https://lore.kernel.org/netdev/CAG48ez0MHBbENX5gCdHAUXZ7h7s20LnepBF-pa5M=7Bi-jZrEA@xxxxxxxxxxxxxx/ > Reported-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> > Link: https://lore.kernel.org/netdev/20220315113841.GA22337@xxxxxxxxxxxxxx/ > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > Cc: Oliver Neukum <oneukum@xxxxxxxx> Acked-by: Oliver Neukum <oneukum@xxxxxxxx>
