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

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 allo=
cated during device start.
- [Medium] In-flight TX URBs and their transfer buffers are permanently lea=
ked when the CAN interface is brought down.
--

--- Patch [11]: [PATCH net 11/20] can: peak_usb: peak_usb_start(): fix doub=
le free of transfer buffer on URB submit error ---
commit 0de6a57bc3019e9140f22ba4065f241bf08735cd
Author: Maoyi Xie <[email protected]>

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.169534517937=
[email protected]/T/#u
Link: https://patch.msgid.link/178163373110.2507866.216458825145756798@maoy=
ixie.com

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/c=
an/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);
> =20
>  			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 =3D=3D NULL and
echo_index =3D=3D 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 =3D 0; i < PCAN_USB_MAX_TX_URBS; i++) {
        ...
		urb =3D usb_alloc_urb(0, GFP_KERNEL);
		if (!urb) {
			err =3D -ENOMEM;
			break;
		}
        ...
	}

	/* warn if we were not able to allocate enough tx contexts */
	if (i < PCAN_USB_MAX_TX_URBS) {
		if (i =3D=3D 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 co=
unt,
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 =3D 0; i < PCAN_USB_MAX_TX_URBS; i++)
		if (dev->tx_contexts[i].echo_index =3D=3D PCAN_USB_MAX_TX_URBS) {
			context =3D dev->tx_contexts + i;
			break;
		}

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

	urb =3D context->urb;
	obuf =3D 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 !=3D 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 =3D dev->tx_contexts + i;
		context->dev =3D dev;
		context->urb =3D urb;
    ...
}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729102802.5051=
[email protected]?part=3D11