Re: [PATCH net] can: peak_usb: 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] In-flight TX URBs and their transfer buffers are permanently leaked when the CAN interface is brought down.
- [High] If `peak_usb_start()` fails to allocate the full set of TX URBs, uninitialized contexts cause a NULL pointer dereference in `peak_usb_ndo_start_xmit()`.
--

commit 940b179b096bd59f1b3c9ac845bb1de2207ae9fa
Author: Maoyi Xie <maoyixie.tju@gmail.com>

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

This commit removes a redundant explicit free of the transfer buffer in
the error path of peak_usb_start() because usb_free_urb() already handles
it when URB_FREE_BUFFER is set.

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

> 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 cf48bb26d46d2..079b23dd74f04 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 driver leak in-flight TX URBs
and their transfer buffers when the CAN interface is brought down?

When the interface is stopped, peak_usb_ndo_stop() calls
peak_usb_unlink_all_urbs() to clean up. This function iterates over the
pre-allocated TX URBs but explicitly skips freeing any URB that is
currently in-flight:

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;
        }

        usb_free_urb(urb);
        dev->tx_contexts[i].urb = NULL;
}

While usb_kill_anchored_urbs() will abort the submitted URBs and cause the
USB core to drop its submission reference, the driver's original allocation
reference is never dropped because of the continue statement above.

When the interface is subsequently restarted, peak_usb_start()
unconditionally allocates new URBs, overwriting the context pointers
and permanently leaking the old URBs.


[Severity: High]
This isn't a bug introduced by this patch, but could this code path cause
a NULL pointer dereference in peak_usb_ndo_start_xmit() if the driver fails
to allocate the full set of TX URBs?

Later in peak_usb_start(), if memory allocation fails for a TX context after
at least one context is successfully allocated, the driver issues a warning
and proceeds:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_start() {
        /* 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");
        }
        ...
        return 0;
}

The unallocated contexts retain a NULL urb pointer and their echo_index 
is set to PCAN_USB_MAX_TX_URBS, marking them as available.

However, peak_usb_ndo_start_xmit() only stops the netdev TX queue when the
total number of active URBs reaches PCAN_USB_MAX_TX_URBS. If the number
of concurrent transmissions exceeds the number of successfully allocated
contexts, it can select an unallocated context and blindly dereference it:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_ndo_start_xmit() {
        if (!context) {
                /* should not occur except during restart */
                return NETDEV_TX_BUSY;
        }

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178163373110.2507866.216458825145756798@maoyixie.com?part=1