Re: [PATCH net 11/20] can: peak_usb: peak_usb_start(): fix double free of transfer buffer on URB submit error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] NULL pointer dereference in TX path if TX URBs were partially allocated during device start.
- [Medium] In-flight TX URBs and their transfer buffers are permanently leaked when the CAN interface is brought down.
--

--- Patch [11]: [PATCH net 11/20] can: peak_usb: peak_usb_start(): fix double free of transfer buffer on URB submit error ---
commit 0de6a57bc3019e9140f22ba4065f241bf08735cd
Author: Maoyi Xie <maoyixie.tju@gmail.com>

can: peak_usb: peak_usb_start(): fix double free of transfer buffer on URB submit error

This patch removes a redundant kfree() in the RX URB error path to prevent a
double-free. Because URB_FREE_BUFFER is set, usb_free_urb() already releases
the transfer buffer.

Closes: https://lore.kernel.org/linux-can/178159320216.2154888.16953451793788581739@maoyixie.com/T/#u
Link: https://patch.msgid.link/178163373110.2507866.216458825145756798@maoyixie.com

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index c7933d1acc99a..55aad01cd8cac 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -470,7 +470,6 @@ static int peak_usb_start(struct peak_usb_device *dev)
>  				netif_device_detach(dev->netdev);
>  
>  			usb_unanchor_urb(urb);
> -			kfree(buf);
>  			usb_free_urb(urb);
>  			break;
>  		}

[Severity: High]
This is a pre-existing issue, but does this code cause a NULL pointer
dereference when accessing urb->transfer_buffer?

If usb_alloc_urb() fails after the first allocation in peak_usb_start(), it
leaves unallocated contexts with urb == NULL and
echo_index == PCAN_USB_MAX_TX_URBS:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_start() {
    ...
	/* pre-alloc tx buffers and corresponding urbs */
	for (i = 0; i < PCAN_USB_MAX_TX_URBS; i++) {
        ...
		urb = usb_alloc_urb(0, GFP_KERNEL);
		if (!urb) {
			err = -ENOMEM;
			break;
		}
        ...
	}

	/* warn if we were not able to allocate enough tx contexts */
	if (i < PCAN_USB_MAX_TX_URBS) {
		if (i == 0) {
			netdev_err(netdev, "couldn't setup any tx URB\n");
			goto err_tx;
		}
		netdev_warn(netdev, "tx performance may be slow\n");
	}
    ...
}

Since the network queue isn't stopped based on the successful allocation count,
can peak_usb_ndo_start_xmit() pick an unallocated context and dereference
a NULL urb pointer?

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_ndo_start_xmit() {
    ...
	for (i = 0; i < PCAN_USB_MAX_TX_URBS; i++)
		if (dev->tx_contexts[i].echo_index == PCAN_USB_MAX_TX_URBS) {
			context = dev->tx_contexts + i;
			break;
		}

	if (!context) {
		/* should not occur except during restart */
		return NETDEV_TX_BUSY;
	}

	urb = context->urb;
	obuf = urb->transfer_buffer;
    ...
}


[Severity: Medium]
This is a pre-existing issue, but does this leak struct urb and
transfer_buffer allocations?

When the interface is brought down, peak_usb_unlink_all_urbs() skips calling
usb_free_urb() on in-flight TX URBs, assuming the USB core will free them:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_unlink_all_urbs() {
    ...
		if (!urb ||
		    dev->tx_contexts[i].echo_index != PCAN_USB_MAX_TX_URBS) {
			/*
			 * this urb is already released or always submitted,
			 * let usb core free by itself
			 */
			continue;
		}
    ...
}

However, the driver holds a base reference from usb_alloc_urb(). When the
interface is brought back up, peak_usb_start() allocates new URBs and
overwrites the pointers, permanently leaking the previous allocations:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_start() {
    ...
		context = dev->tx_contexts + i;
		context->dev = dev;
		context->urb = urb;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=11