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

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

[Peter GJ. Park]

When usbnet_disconnect() queued while usbnet_probe() processing,
it results to free_netdev before kevent gets to run on workqueue,
thus workqueue does assign_work() with referencing freeed memory address.

For graceful disconnect and to prevent use-after-free of netdev pointer,
the fix adds canceling work and timer those are placed by usbnet_probe()

Signed-off-by: Peter GJ. Park <gyujoon.park@samsung.com>
---
 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);
 }
 EXPORT_SYMBOL_GPL(usbnet_disconnect);

---
base-commit: 86731a2a651e58953fc949573895f2fa6d456841
change-id: 20250625-usbnet-uaf-fix-9ab85de1b8cf

Best regards,
-- 
Peter GJ. Park <gyujoon.park@samsung.com>

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

[Paolo Abeni]

On 6/25/25 11:33 AM, Peter GJ. Park wrote:
> When usbnet_disconnect() queued while usbnet_probe() processing,
> it results to free_netdev before kevent gets to run on workqueue,
> thus workqueue does assign_work() with referencing freeed memory address.
> 
> For graceful disconnect and to prevent use-after-free of netdev pointer,
> the fix adds canceling work and timer those are placed by usbnet_probe()
> 
> Signed-off-by: Peter GJ. Park <gyujoon.park@samsung.com>

You should include a suitable fixes tag, and you should have specified
the target tree ('net' in this case) in the prefix subjext

> ---
>  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?

/P

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

[Peter GJ. Park]

When usbnet_disconnect() queued while usbnet_probe() processing,
it results to free_netdev before kevent gets to run on workqueue,
thus workqueue does assign_work() with referencing freeed memory address.

For graceful disconnect and to prevent use-after-free of netdev pointer,
the fix adds canceling work and timer those are placed by usbnet_probe()

Signed-off-by: Peter GJ. Park <gyujoon.park@samsung.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
---
 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 c04e715a4c2a..3c5d9ba7fa66 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);
 }
 EXPORT_SYMBOL_GPL(usbnet_disconnect);
-- 
2.25.1

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

[Peter GJ. Park]

When usbnet_disconnect() queued while usbnet_probe() processing,
it results to free_netdev before kevent gets to run on workqueue,
thus workqueue does assign_work() with referencing freeed memory address.

For graceful disconnect and to prevent use-after-free of netdev pointer,
the fix adds canceling work and timer those are placed by usbnet_probe()

Signed-off-by: Peter GJ. Park <gyujoon.park@samsung.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
---
 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 c04e715a4c2a..3c5d9ba7fa66 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);
 }
 EXPORT_SYMBOL_GPL(usbnet_disconnect);
--
2.25.1

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

[Peter GJ. Park]

[-- Attachment #1: Type: text/plain, Size: 6262 bytes --]

On Thu, Jun 26, 2025 at 11:21:39AM +0200, Paolo Abeni wrote:
> On 6/25/25 11:33 AM, Peter GJ. Park wrote:
> > When usbnet_disconnect() queued while usbnet_probe() processing,
> > it results to free_netdev before kevent gets to run on workqueue,
> > thus workqueue does assign_work() with referencing freeed memory address.
> >
> > For graceful disconnect and to prevent use-after-free of netdev pointer,
> > the fix adds canceling work and timer those are placed by usbnet_probe()
> >
> > Signed-off-by: Peter GJ. Park <gyujoon.park@samsung.com>
>
> You should include a suitable fixes tag, and you should have specified
> the target tree ('net' in this case) in the prefix subjext
>
It has been applied with v2 patch. Thank you for this.
> > ---
> >  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?
>
> /P
>
This uaf detected while my syzkaller project for usb kernel driver.

First hub_event work:usb_new_device arrived and processing in wq,
While second new hub_event work:usb_disconnect put into wq.
Then third usbnet:kevent work put by first hub_event work.

Dev pointer free-ed by second work without preceding usbnet_stop(), so the usbnetkevent still queued in wq,
Finally it detected by kasan as use-after-free when wq try to access that poiter.

Here is log snippet.

[ 3041.949116] [5:    kworker/5:1:  130] BUG: KASAN: slab-use-after-free in assign_work+0xe8/0x280
[ 3041.949128] [5:    kworker/5:1:  130] Read of size 8 at addr ffffff88ef60ece8 by task kworker/5:1/130
...
[ 3041.949155] [5:    kworker/5:1:  130] Workqueue:  0x0 (usb_hub_wq)
[ 3041.949167] [5:    kworker/5:1:  130] Call trace:
[ 3041.949170] [5:    kworker/5:1:  130]  dump_backtrace+0x120/0x170
[ 3041.949178] [5:    kworker/5:1:  130]  show_stack+0x2c/0x40
[ 3041.949184] [5:    kworker/5:1:  130]  dump_stack_lvl+0x68/0x84
[ 3041.949193] [5:    kworker/5:1:  130]  print_report+0x13c/0x6f8
[ 3041.949203] [5:    kworker/5:1:  130]  kasan_report+0xdc/0x13c
[ 3041.949211] [5:    kworker/5:1:  130]  __asan_load8+0x98/0xa0
[ 3041.949216] [5:    kworker/5:1:  130]  assign_work+0xe8/0x280
[ 3041.949224] [5:    kworker/5:1:  130]  worker_thread+0x458/0x670
[ 3041.949231] [5:    kworker/5:1:  130]  kthread+0x1d8/0x298
[ 3041.949238] [5:    kworker/5:1:  130]  ret_from_fork+0x10/0x20

[ 3041.949248] [5:    kworker/5:1:  130] Allocated by task 130:
...
[ 3041.949275] [5:    kworker/5:1:  130]  __kmalloc_node+0x74/0x194
[ 3041.949282] [5:    kworker/5:1:  130]  kvmalloc_node+0x160/0x394
[ 3041.949289] [5:    kworker/5:1:  130]  alloc_netdev_mqs+0x6c/0x718
[ 3041.949298] [5:    kworker/5:1:  130]  alloc_etherdev_mqs+0x48/0x60
[ 3041.949305] [5:    kworker/5:1:  130]  usbnet_probe+0xd8/0xf10 [usbnet]
[ 3041.949348] [5:    kworker/5:1:  130]  usb_probe_interface+0x338/0x4fc
...
[ 3041.949516] [5:    kworker/5:1:  130]  usb_new_device+0x7c8/0xbdc
[ 3041.949523] [5:    kworker/5:1:  130]  hub_event+0x1808/0x2564
[ 3041.949530] [5:    kworker/5:1:  130]  process_scheduled_works+0x3cc/0x92c
[ 3041.949538] [5:    kworker/5:1:  130]  worker_thread+0x468/0x670
[ 3041.949546] [5:    kworker/5:1:  130]  kthread+0x1d8/0x298
[ 3041.949552] [5:    kworker/5:1:  130]  ret_from_fork+0x10/0x20

[ 3041.949561] [5:    kworker/5:1:  130] Freed by task 130:
...
[ 3041.949604] [5:    kworker/5:1:  130]  __kmem_cache_free+0xa0/0x260
[ 3041.949610] [5:    kworker/5:1:  130]  kfree+0x60/0x124
[ 3041.949617] [5:    kworker/5:1:  130]  kvfree+0x40/0x54
[ 3041.949622] [5:    kworker/5:1:  130]  netdev_freemem+0x2c/0x40
[ 3041.949630] [5:    kworker/5:1:  130]  netdev_release+0x50/0x6c
[ 3041.949638] [5:    kworker/5:1:  130]  device_release+0x74/0x13c
[ 3041.949645] [5:    kworker/5:1:  130]  kobject_put+0xfc/0x1a8
[ 3041.949654] [5:    kworker/5:1:  130]  put_device+0x28/0x40
[ 3041.949660] [5:    kworker/5:1:  130]  free_netdev+0x234/0x284
[ 3041.949667] [5:    kworker/5:1:  130]  usbnet_disconnect+0x18c/0x250 [usbnet]
[ 3041.949706] [5:    kworker/5:1:  130]  usb_unbind_interface+0x130/0x414
[ 3041.949713] [5:    kworker/5:1:  130]  device_release_driver_internal+0x2e8/0x494
[ 3041.949721] [5:    kworker/5:1:  130]  device_release_driver+0x28/0x3c
[ 3041.949730] [5:    kworker/5:1:  130]  bus_remove_device+0x250/0x268
[ 3041.949737] [5:    kworker/5:1:  130]  device_del+0x304/0x530
[ 3041.949744] [5:    kworker/5:1:  130]  usb_disable_device+0x1d8/0x340
[ 3041.949750] [5:    kworker/5:1:  130]  usb_disconnect+0x1c4/0x4f0
[ 3041.949757] [5:    kworker/5:1:  130]  hub_event+0x1200/0x2564
[ 3041.949764] [5:    kworker/5:1:  130]  process_scheduled_works+0x3cc/0x92c
[ 3041.949772] [5:    kworker/5:1:  130]  worker_thread+0x468/0x670
[ 3041.949780] [5:    kworker/5:1:  130]  kthread+0x1d8/0x298
[ 3041.949786] [5:    kworker/5:1:  130]  ret_from_fork+0x10/0x20

[ 3041.949796] [5:    kworker/5:1:  130] Last potentially related work creation:
...
[ 3041.949820] [5:    kworker/5:1:  130]  __queue_work+0x5d0/0xaf4
[ 3041.949827] [5:    kworker/5:1:  130]  queue_work_on+0x64/0xb0
[ 3041.949833] [5:    kworker/5:1:  130]  usbnet_link_change+0xdc/0x13c [usbnet]
[ 3041.949873] [5:    kworker/5:1:  130]  usbnet_probe+0xe5c/0xf10 [usbnet]
...
[ 3041.950078] [5:    kworker/5:1:  130]  usb_new_device+0x7c8/0xbdc
[ 3041.950085] [5:    kworker/5:1:  130]  hub_event+0x1808/0x2564
[ 3041.950092] [5:    kworker/5:1:  130]  process_scheduled_works+0x3cc/0x92c
[ 3041.950100] [5:    kworker/5:1:  130]  worker_thread+0x468/0x670
[ 3041.950107] [5:    kworker/5:1:  130]  kthread+0x1d8/0x298
[ 3041.950113] [5:    kworker/5:1:  130]  ret_from_fork+0x10/0x20

Best Regards,
GJ

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Oliver Neukum]

Hi,

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?

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.

	TIA
		Oliver

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

[Jakub Kicinski]

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;
 

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

[Jakub Kicinski]

On Fri, 27 Jun 2025 19:59:53 +0900 Peter GJ. Park wrote:
> When usbnet_disconnect() queued while usbnet_probe() processing,
> it results to free_netdev before kevent gets to run on workqueue,
> thus workqueue does assign_work() with referencing freeed memory address.
> 
> For graceful disconnect and to prevent use-after-free of netdev pointer,
> the fix adds canceling work and timer those are placed by usbnet_probe()

Discussion on the v1 thread is ongoing, please repost once it concludes.
-- 
pw-bot: cr

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

[Oliver Neukum]

On 02.07.25 03:26, Jakub Kicinski wrote:
> On Tue, 1 Jul 2025 15:22:54 +0200 Oliver Neukum wrote:

>> 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.

Thank you for clarifying that.

>> 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:

It turns out that user space wants to know whether there is carrier
even before it uses an interface because it uses that information
to decide whether to use the link.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=444043

However, it looks to me like the issue is specifically
queuing work for kevent. That would call for reverting
0162c55463057 ("usbnet: apply usbnet_link_change")
[taking author into CC]

	Regards
		Oliver

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

[Jakub Kicinski]

On Wed, 2 Jul 2025 12:54:23 +0200 Oliver Neukum wrote:
> >> 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:  
> 
> It turns out that user space wants to know whether there is carrier
> even before it uses an interface because it uses that information
> to decide whether to use the link.
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=444043

Ah. We should totally move the carrier clear _prior_ to registering 
the netdev!

> However, it looks to me like the issue is specifically
> queuing work for kevent. That would call for reverting
> 0162c55463057 ("usbnet: apply usbnet_link_change")
> [taking author into CC]

Hm, spying on git logs I think Ming Lei changed employers from
Cannonical to RedHat in early 2017. Adding the @redhat address.