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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220621164316.GA8969@wunner.de>
Date:   Tue, 21 Jun 2022 18:43:16 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Oliver Neukum <oneukum@...e.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org, Dan Williams <dan.j.williams@...el.com>,
        Jann Horn <jannh@...gle.com>
Subject: Re: [PATCH net] sierra_net: Fix use-after-free on unbind

[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