[adding Jann as UAF connoisseur to cc] On Tue, Jun 14, 2022 at 12:48:23PM +0200, Oliver Neukum wrote: > On 14.06.22 10:50, Lukas Wunner wrote: > > @@ -758,6 +758,8 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf) > > > > dev_dbg(&dev->udev->dev, "%s", __func__); > > > > + usbnet_status_stop(dev); > > + > > /* kill the timer and work */ > > del_timer_sync(&priv->sync_timer); > > cancel_work_sync(&priv->sierra_net_kevent); > > as far as I can see the following race condition exists: > > CPU A: > intr_complete() -> static void sierra_net_status() -> defer_kevent() > > CPU B: > usbnet_stop_status() ---- kills the URB but only the URB, kevent scheduled > > CPU A: > sierra_net_kevent -> sierra_net_dosync() -> > > CPU B: > -> del_timer_sync(&priv->sync_timer); ---- NOP, too early > > CPU A: > add_timer(&priv->sync_timer); > > CPU B: > cancel_work_sync(&priv->sierra_net_kevent); ---- NOP, too late I see your point, but what's the solution? I could call netif_device_detach() on ->disconnect(), then avoid scheduling sierra_net_kevent in the timer if !netif_device_present(), and also avoid arming the timer in sierra_net_kevent under the same condition. Still, I think I'd need 3 calls to make this bulletproof, either del_timer_sync(&priv->sync_timer); cancel_work_sync(&priv->sierra_net_kevent); del_timer_sync(&priv->sync_timer); or cancel_work_sync(&priv->sierra_net_kevent); del_timer_sync(&priv->sync_timer); cancel_work_sync(&priv->sierra_net_kevent); Doesn't really matter which of these two. Am I right? Is there a better (simpler) approach? FWIW, the logic in usbnet.c looks similarly flawed: There's a timer, a tasklet and a work. (Sounds like one of those "... walk into a bar" jokes.) The timer is armed by the tx/rx URB completion callbacks. Those URBs are terminated in usbnet_stop() and the timer is deleted. So far so good. But: The tasklet schedules the work. The work schedules the tasklet. The tasklet also schedules itself. We kill the tasklet in usbnet_stop() and afterwards cancel the work in usbnet_disconnect(). What happens if the work schedules the tasklet again? Another UAF. That may happen in the EVENT_RX_HALT, EVENT_RX_MEMORY, EVENT_LINK_RESET and EVENT_LINK_CHANGE code paths. A few netif_device_present() safeguards may help to prevent rescheduling the killed tasklet, but I suspect we may again need 3 calls here (tasklet_kill() / cancel_work_sync() / tasklet_kill()) to make it bulletproof. What do you think? As a heads-up, I'm going to move the cancel_work_sync() to usbnet_stop() in an upcoming patch. That seems to be Jakub's preferred approach to tackle the linkwatch UAF. I fear it may increase the risk of encountering the issues outlined above as the time between tasklet_kill() and cancel_work_sync() is reduced: https://github.com/l1k/linux/commit/89988b499ab9 Thanks, Lukas
