Re: [PATCH] can: usb: f81604: fix struct f81604_int_data size mismatch

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Improper teardown ordering in f81604_close() causes a use-after-free of the driver's private data via clear_reg_work.
- [High] Unbounded memory and URB resource leak during CAN bus-off recovery and interface toggle.
- [High] Soft lockup (CPU Denial of Service) due to bypassed URB status checks in the interrupt callback.
--

--- Patch [1]: [PATCH] can: usb: f81604: fix struct f81604_int_data size mismatch ---
commit 53e24ec343d9d1cbf17bed31729c1ccebdbe512a
Author: PS10 PETER HONG 洪繼澤 <peter_hong@fintek.com.tw>

can: usb: f81604: fix struct f81604_int_data size mismatch

The struct f81604_int_data was padded to 12 bytes but hardware sends exactly
9 bytes, causing short URB checks to discard valid messages and resulting in
transmission failures. This commit removes the padding attribute so the struct
size matches the 9-byte hardware data length.

> diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> index f12318268e46b..4c147b9d6d690 100644
> --- a/drivers/net/can/usb/f81604.c
> +++ b/drivers/net/can/usb/f81604.c
> @@ -169,7 +169,7 @@ struct f81604_int_data {
>  	u8 rxerr;
>  	u8 txerr;
>  	u8 val;
> -} __packed __aligned(4);
> +} __packed;
>  
>  struct f81604_sff {
>  	__be16 id;

[Severity: High]
This isn't a bug introduced by this patch, but is the teardown ordering in
f81604_close() correct?

drivers/net/can/usb/f81604.c:f81604_close() {
...
	cancel_work_sync(&priv->clear_reg_work);
	close_candev(netdev);

	f81604_unregister_urbs(priv);
...
}

Because f81604_unregister_urbs() is called after cancel_work_sync(), URBs are
still active. A concurrently executing interrupt URB callback can schedule the
work again via f81604_handle_can_bus_errors():

drivers/net/can/usb/f81604.c:f81604_handle_can_bus_errors() {
...
	if (priv->clear_flags)
		schedule_work(&priv->clear_reg_work);
...
}

When the device is later disconnected, f81604_disconnect() frees the netdev
and its embedded priv structure. Will the pending clear_reg_work then execute
and access the freed priv pointer, leading to a memory corruption or crash?

[Severity: High]
This is a pre-existing issue, but does the driver leak URBs and their allocated
buffers during bus-off recovery and interface toggles?

When the CAN bus recovers from a bus-off condition, f81604_set_mode() calls
f81604_start() which registers new URBs:

drivers/net/can/usb/f81604.c:f81604_set_mode() {
	switch (mode) {
	case CAN_MODE_START:
		ret = f81604_start(netdev);
...
}

f81604_start() blindly calls f81604_register_urbs(), allocating and submitting
5 new URBs. The old URBs were never killed because can_bus_off() does not stop
them, seemingly leaking 5 URBs on every recovery.

Additionally, during f81604_close(), if an URB callback is executing
concurrently, it checks netif_device_present() instead of netif_running():

drivers/net/can/usb/f81604.c:f81604_read_bulk_callback() {
...
	if (!netif_device_present(netdev))
		return;
...
resubmit_urb:
	usb_anchor_urb(urb, &priv->urbs_anchor);
	ret = usb_submit_urb(urb, GFP_ATOMIC);
...
}

Will this cause the URB to be resubmitted even after ndo_stop has run, and when
the interface is brought back up, multiply the number of active URBs leading to
resource exhaustion?

[Severity: High]
This is a pre-existing issue, but can the short length check in the interrupt
callback cause a soft lockup?

In f81604_read_int_callback(), the short URB check is performed before the
status check:

drivers/net/can/usb/f81604.c:f81604_read_int_callback() {
...
	if (urb->actual_length < sizeof(*data)) {
		netdev_warn(netdev, "%s: short int URB: %u < %zu\n",
			    __func__, urb->actual_length, sizeof(*data));
		goto resubmit_urb;
	}

	switch (urb->status) {
...
}

When a USB hardware error occurs, the host controller completes the URB with an
error status and an actual_length of 0. Because 0 < 9, the code logs a warning
and jumps straight to resubmit_urb, bypassing the error-handling switch.

Will the host controller then immediately fail the resubmitted URB again,
triggering an infinite loop of hardware errors in softirq context and leading
to a CPU soft lockup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824082758.9118-1-peter_hong@fintek.com.tw?part=1