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@xxxxxxxxxxxx/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