peak_usb_start(): double free of RX buffer when usb_submit_urb() fails

peak_usb_start(): double free of RX buffer when usb_submit_urb() fails

[Maoyi Xie]

Hi all,

I was going over the USB CAN drivers that use URB_FREE_BUFFER after the lan78xx
double-free fix, and I think peak_usb_start() in
drivers/net/can/usb/peak_usb/pcan_usb_core.c can free an RX transfer buffer
twice on the usb_submit_urb() error path. I am not totally sure, so I would
appreciate it if you could let me know whether you see this as a real problem.

This is the relevant part of the RX URB loop in 7.1-rc7.

	buf = kmalloc(dev->adapter->rx_buffer_size, GFP_KERNEL);
	...
	usb_fill_bulk_urb(urb, dev->udev,
		usb_rcvbulkpipe(dev->udev, dev->ep_msg_in),
		buf, dev->adapter->rx_buffer_size,
		peak_usb_read_bulk_callback, dev);

	/* ask last usb_free_urb() to also kfree() transfer_buffer */
	urb->transfer_flags |= URB_FREE_BUFFER;
	usb_anchor_urb(urb, &dev->rx_submitted);

	err = usb_submit_urb(urb, GFP_KERNEL);
	if (err) {
		if (err == -ENODEV)
			netif_device_detach(dev->netdev);

		usb_unanchor_urb(urb);
		kfree(buf);
		usb_free_urb(urb);
		break;
	}

URB_FREE_BUFFER is set on the URB before submit, so buf belongs to the URB. When
the URB is finally destroyed, urb_destroy() frees that buffer for us with

	if (urb->transfer_flags & URB_FREE_BUFFER)
		kfree(urb->transfer_buffer);

So in the error path kfree(buf) frees the buffer once, and then usb_free_urb(urb)
frees the same buffer a second time through urb_destroy(). That looks like a
double free of the kmalloc'd RX buffer.

The error path is taken when usb_submit_urb() returns non-zero, e.g. -ENODEV if
the device is removed while the CAN interface is being brought up (ip link set
canX up), or -ENOMEM / endpoint errors. A device that disconnects at the wrong
moment, or otherwise makes the submit fail, would hit it.

I do not have a PCAN-USB adapter, so I reproduced just the buffer handling on
7.1-rc7 under KASAN. A small harness replays the exact sequence above
(URB_FREE_BUFFER set, then kfree(buf), then usb_free_urb(urb)), and KASAN reports
the second free.

  BUG: KASAN: double-free in usb_free_urb.part.0+0x91/0xb0
  Free of addr ffff8881069ccb80 by task trigger.sh/285
   kfree+0x113/0x3c0
   usb_free_urb.part.0+0x91/0xb0          <- second free, via urb_destroy()
  ...
  Freed by task 285:
   kfree+0x137/0x3c0                       <- first free, the explicit kfree(buf)
  ...
  The buggy address belongs to the object at ffff8881069ccb80
   which belongs to the cache kmalloc-64 of size 64

(The harness allocates a 64-byte buffer, which is why KASAN shows kmalloc-64,
while the real driver allocates dev->adapter->rx_buffer_size.) The first free is
the explicit kfree(buf), and the second is usb_free_urb() going through
urb_destroy() and freeing urb->transfer_buffer because URB_FREE_BUFFER is set.

The smallest fix I could see is to drop the redundant kfree(buf), since
usb_free_urb() already releases the buffer. This is what commit 03819abbeb11
("net: usb: lan78xx: Fix double free issue with interrupt buffer allocation")
did for the same pattern.

	 		usb_unanchor_urb(urb);
	-		kfree(buf);
	 		usb_free_urb(urb);
	 		break;

With that one-line change the same harness frees the buffer exactly once and the
KASAN report is gone.

It looks like this has been there since the driver was first merged
(bb4785551f64), so if it is a real bug it would be a stable candidate.

I would appreciate it if you could let me know whether you agree this is a real
double free and whether dropping the kfree(buf) is the right fix. I am happy to
send a proper patch with the reproducer once you confirm.

Thanks,
Maoyi
https://maoyixie.com/

Re: peak_usb_start(): double free of RX buffer when usb_submit_urb() fails

[Vincent Mailhol]

On 16/06/2026 at 09:00, Maoyi Xie wrote:
> Hi all,
> 
> I was going over the USB CAN drivers that use URB_FREE_BUFFER after the lan78xx
> double-free fix, and I think peak_usb_start() in
> drivers/net/can/usb/peak_usb/pcan_usb_core.c can free an RX transfer buffer
> twice on the usb_submit_urb() error path. I am not totally sure, so I would
> appreciate it if you could let me know whether you see this as a real problem.
> 
> This is the relevant part of the RX URB loop in 7.1-rc7.
> 
> 	buf = kmalloc(dev->adapter->rx_buffer_size, GFP_KERNEL);
> 	...
> 	usb_fill_bulk_urb(urb, dev->udev,
> 		usb_rcvbulkpipe(dev->udev, dev->ep_msg_in),
> 		buf, dev->adapter->rx_buffer_size,
> 		peak_usb_read_bulk_callback, dev);
> 
> 	/* ask last usb_free_urb() to also kfree() transfer_buffer */
> 	urb->transfer_flags |= URB_FREE_BUFFER;
> 	usb_anchor_urb(urb, &dev->rx_submitted);
> 
> 	err = usb_submit_urb(urb, GFP_KERNEL);
> 	if (err) {
> 		if (err == -ENODEV)
> 			netif_device_detach(dev->netdev);
> 
> 		usb_unanchor_urb(urb);
> 		kfree(buf);
> 		usb_free_urb(urb);
> 		break;
> 	}
> 
> URB_FREE_BUFFER is set on the URB before submit, so buf belongs to the URB. When
> the URB is finally destroyed, urb_destroy() frees that buffer for us with
> 
> 	if (urb->transfer_flags & URB_FREE_BUFFER)
> 		kfree(urb->transfer_buffer);
> 
> So in the error path kfree(buf) frees the buffer once, and then usb_free_urb(urb)
> frees the same buffer a second time through urb_destroy(). That looks like a
> double free of the kmalloc'd RX buffer.
> 
> The error path is taken when usb_submit_urb() returns non-zero, e.g. -ENODEV if
> the device is removed while the CAN interface is being brought up (ip link set
> canX up), or -ENOMEM / endpoint errors. A device that disconnects at the wrong
> moment, or otherwise makes the submit fail, would hit it.
> 
> I do not have a PCAN-USB adapter, so I reproduced just the buffer handling on
> 7.1-rc7 under KASAN. A small harness replays the exact sequence above
> (URB_FREE_BUFFER set, then kfree(buf), then usb_free_urb(urb)), and KASAN reports
> the second free.
> 
>   BUG: KASAN: double-free in usb_free_urb.part.0+0x91/0xb0
>   Free of addr ffff8881069ccb80 by task trigger.sh/285
>    kfree+0x113/0x3c0
>    usb_free_urb.part.0+0x91/0xb0          <- second free, via urb_destroy()
>   ...
>   Freed by task 285:
>    kfree+0x137/0x3c0                       <- first free, the explicit kfree(buf)
>   ...
>   The buggy address belongs to the object at ffff8881069ccb80
>    which belongs to the cache kmalloc-64 of size 64
> 
> (The harness allocates a 64-byte buffer, which is why KASAN shows kmalloc-64,
> while the real driver allocates dev->adapter->rx_buffer_size.) The first free is
> the explicit kfree(buf), and the second is usb_free_urb() going through
> urb_destroy() and freeing urb->transfer_buffer because URB_FREE_BUFFER is set.
> 
> The smallest fix I could see is to drop the redundant kfree(buf), since
> usb_free_urb() already releases the buffer. This is what commit 03819abbeb11
> ("net: usb: lan78xx: Fix double free issue with interrupt buffer allocation")
> did for the same pattern.
> 
> 	 		usb_unanchor_urb(urb);
> 	-		kfree(buf);
> 	 		usb_free_urb(urb);
> 	 		break;
> 
> With that one-line change the same harness frees the buffer exactly once and the
> KASAN report is gone.
> 
> It looks like this has been there since the driver was first merged
> (bb4785551f64), so if it is a real bug it would be a stable candidate.
> 
> I would appreciate it if you could let me know whether you agree this is a real
> double free and whether dropping the kfree(buf) is the right fix. I am happy to
> send a proper patch with the reproducer once you confirm.

Thanks for the well written report. I looked at the code, and I can
confirm that usb_free_urb() calls usb_destroy() if no one is using the
urb anymore. And finally usb_destroy() calls

  kfree(urb->transfer_buffer);

leading in the double free.

So, same conclusion as you. You can send a patch. Now that you sent a
report, don't forget to add the Closes tag to your patch:

Closes: https://lore.kernel.org/linux-can/178159320216.2154888.16953451793788581739@maoyixie.com/T/#u

Not sure that you need to include the full reproducer in your patch
message. The BUG: KASAN log is enough. But if you want, you can still
post your reproducer in this thread.


Yours sincerely,
Vincent Mailhol