Re: [PATCH net] sierra_net: Fix use-after-free on unbind

[Lukas Wunner <lukas@wunner.de>]

[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