Re: [PATCH] net: usb: usbnet: fix use-after-free in race on workqueue

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 1 Jul 2025 15:22:54 +0200 Oliver Neukum wrote:
> On 26.06.25 11:21, Paolo Abeni wrote:
> >>   drivers/net/usb/usbnet.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> >> index c04e715a4c2ade3bc5587b0df71643a25cf88c55..3c5d9ba7fa6660273137c80106746103f84f5a37 100644
> >> --- a/drivers/net/usb/usbnet.c
> >> +++ b/drivers/net/usb/usbnet.c
> >> @@ -1660,6 +1660,9 @@ void usbnet_disconnect (struct usb_interface *intf)
> >>   	usb_free_urb(dev->interrupt);
> >>   	kfree(dev->padding_pkt);
> >>   
> >> +	timer_delete_sync(&dev->delay);
> >> +	tasklet_kill(&dev->bh);
> >> +	cancel_work_sync(&dev->kevent);
> >>   	free_netdev(net);  
> > This happens after unregister_netdev(), which calls usbnet_stop() that
> > already performs the above cleanup. How the race is supposed to take place?  
> 
> That is indeed a core question, which we really need an answer to.
> Do I interpret dev_close_many() correctly, if I state that the
> ndo_stop() method will _not_ be called if the device has never been
> opened?

Correct, open and close are paired. Most drivers would crash if we
tried to close them before they ever got opened. 

> I am sorry to be a stickler here, but if that turns out to be true,
> usbnet is not the only driver that has this bug.

Shooting from the hip slightly, but its unusual for a driver to start
link monitoring before open. After all there can be no packets on a
device that's closed. Why not something like:

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9564478a79cc..b75b0b5c3abc 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -896,6 +896,9 @@ int usbnet_open (struct net_device *net)
        int                     retval;
        const struct driver_info *info = dev->driver_info;
 
+       if (dev->driver_info->flags & FLAG_LINK_INTR)
+               usbnet_link_change(dev, 0, 0);
+
        if ((retval = usb_autopm_get_interface(dev->intf)) < 0) {
                netif_info(dev, ifup, dev->net,
                           "resumption fail (%d) usbnet usb-%s-%s, %s\n",
@@ -1862,8 +1865,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
        netif_device_attach (net);
 
-       if (dev->driver_info->flags & FLAG_LINK_INTR)
-               usbnet_link_change(dev, 0, 0);
+       netif_carrier_off(net);
 
        return 0;