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/