* Re: [PATCH v2] can: usb: f81604: fix struct f81604_int_data size mismatch
2026-08-24 13:18 [PATCH v2] can: usb: f81604: fix struct f81604_int_data size mismatch Ji-Ze Hong via B4 Relay
@ 2026-08-24 13:26 ` Greg Kroah-Hartman
2026-08-24 15:11 ` Marc Kleine-Budde
2026-10-03 11:45 ` Marc Kleine-Budde
2 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2026-08-24 13:26 UTC (permalink / raw)
To: peter_hong
Cc: Marc Kleine-Budde, Vincent Mailhol, linux-can, linux-kernel,
stable, Dynetrex, Admin
On Mon, Aug 24, 2026 at 09:18:14PM +0800, Ji-Ze Hong via B4 Relay wrote:
> From: "Ji-Ze Hong (Peter Hong)" <peter_hong@fintek.com.tw>
>
> The struct f81604_int_data defines 9 bytes of interrupt data:
> - Byte 0: Status register (sr)
> - Byte 1: Interrupt register (isrc)
> - Byte 2: Interrupt enable register (ier)
> - Byte 3: Arbitration lost capture (alc)
> - Byte 4: Error code capture (ecc)
> - Byte 5: Error warning limit register (ewlr)
> - Byte 6: RX error counter (rxerr)
> - Byte 7: TX error counter (txerr)
> - Byte 8: Reserved (val)
>
> The hardware sends exactly 9 bytes for the interrupt endpoint.
> However, the struct was defined with __aligned(4) attribute which
> caused the compiler to pad the struct to 12 bytes.
>
> This causes a problem in f81604_read_int_callback() where the short
> URB check compares urb->actual_length against sizeof(*data). When
> sizeof(struct f81604_int_data) is 12 but the hardware only sends 9
> bytes, the check fails and valid interrupt messages are discarded.
>
> This results in the driver only being able to transmit once because
> the TX complete interrupt is never processed.
>
> Fix this by removing the __aligned(4) attribute so the struct size
> matches the actual hardware data size of 9 bytes.
>
> Fixes: 7299b1b39a25 ("can: usb: f81604: handle short interrupt urb messages properly")
> Cc: stable@vger.kernel.org
> Reported-by: Dynetrex, Admin <admin@dynetrex.com>
> Closes: https://lore.kernel.org/all/A3834A07-5639-4779-844F-C5843DFC3928@dynetrex.com/
> Signed-off-by: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
> ---
> v2:
> - Added Reported-by and Remove mismatched Fixes tags
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] can: usb: f81604: fix struct f81604_int_data size mismatch
2026-08-24 13:18 [PATCH v2] can: usb: f81604: fix struct f81604_int_data size mismatch Ji-Ze Hong via B4 Relay
2026-08-24 13:26 ` Greg Kroah-Hartman
@ 2026-08-24 15:11 ` Marc Kleine-Budde
2026-08-24 23:39 ` Drew Willey
2026-10-03 11:45 ` Marc Kleine-Budde
2 siblings, 1 reply; 5+ messages in thread
From: Marc Kleine-Budde @ 2026-08-24 15:11 UTC (permalink / raw)
To: Ji-Ze Hong via B4 Relay
Cc: Vincent Mailhol, Greg Kroah-Hartman, linux-can, linux-kernel,
stable, Dynetrex, Admin, Ji-Ze Hong (Peter Hong)
[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]
On 24.08.2026 21:18:14, Ji-Ze Hong via B4 Relay wrote:
> From: "Ji-Ze Hong (Peter Hong)" <peter_hong@fintek.com.tw>
>
> The struct f81604_int_data defines 9 bytes of interrupt data:
> - Byte 0: Status register (sr)
> - Byte 1: Interrupt register (isrc)
> - Byte 2: Interrupt enable register (ier)
> - Byte 3: Arbitration lost capture (alc)
> - Byte 4: Error code capture (ecc)
> - Byte 5: Error warning limit register (ewlr)
> - Byte 6: RX error counter (rxerr)
> - Byte 7: TX error counter (txerr)
> - Byte 8: Reserved (val)
>
> The hardware sends exactly 9 bytes for the interrupt endpoint.
> However, the struct was defined with __aligned(4) attribute which
> caused the compiler to pad the struct to 12 bytes.
>
> This causes a problem in f81604_read_int_callback() where the short
> URB check compares urb->actual_length against sizeof(*data). When
> sizeof(struct f81604_int_data) is 12 but the hardware only sends 9
> bytes, the check fails and valid interrupt messages are discarded.
>
> This results in the driver only being able to transmit once because
> the TX complete interrupt is never processed.
>
> Fix this by removing the __aligned(4) attribute so the struct size
> matches the actual hardware data size of 9 bytes.
>
> Fixes: 7299b1b39a25 ("can: usb: f81604: handle short interrupt urb messages properly")
> Cc: stable@vger.kernel.org
> Reported-by: Dynetrex, Admin <admin@dynetrex.com>
> Closes: https://lore.kernel.org/all/A3834A07-5639-4779-844F-C5843DFC3928@dynetrex.com/
> Signed-off-by: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
Applied to linux-can.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] can: usb: f81604: fix struct f81604_int_data size mismatch
2026-08-24 13:18 [PATCH v2] can: usb: f81604: fix struct f81604_int_data size mismatch Ji-Ze Hong via B4 Relay
2026-08-24 13:26 ` Greg Kroah-Hartman
2026-08-24 15:11 ` Marc Kleine-Budde
@ 2026-10-03 11:45 ` Marc Kleine-Budde
2 siblings, 0 replies; 5+ messages in thread
From: Marc Kleine-Budde @ 2026-10-03 11:45 UTC (permalink / raw)
To: Ji-Ze Hong via B4 Relay
Cc: Vincent Mailhol, Greg Kroah-Hartman, linux-can, linux-kernel,
stable, Dynetrex, Admin, Ji-Ze Hong (Peter Hong)
[-- Attachment #1: Type: text/plain, Size: 6465 bytes --]
On 24.08.2026 21:18:14, Ji-Ze Hong via B4 Relay wrote:
> From: "Ji-Ze Hong (Peter Hong)" <peter_hong@fintek.com.tw>
>
> The struct f81604_int_data defines 9 bytes of interrupt data:
> - Byte 0: Status register (sr)
> - Byte 1: Interrupt register (isrc)
> - Byte 2: Interrupt enable register (ier)
> - Byte 3: Arbitration lost capture (alc)
> - Byte 4: Error code capture (ecc)
> - Byte 5: Error warning limit register (ewlr)
> - Byte 6: RX error counter (rxerr)
> - Byte 7: TX error counter (txerr)
> - Byte 8: Reserved (val)
>
> The hardware sends exactly 9 bytes for the interrupt endpoint.
> However, the struct was defined with __aligned(4) attribute which
> caused the compiler to pad the struct to 12 bytes.
>
> This causes a problem in f81604_read_int_callback() where the short
> URB check compares urb->actual_length against sizeof(*data). When
> sizeof(struct f81604_int_data) is 12 but the hardware only sends 9
> bytes, the check fails and valid interrupt messages are discarded.
>
> This results in the driver only being able to transmit once because
> the TX complete interrupt is never processed.
>
> Fix this by removing the __aligned(4) attribute so the struct size
> matches the actual hardware data size of 9 bytes.
>
> Fixes: 7299b1b39a25 ("can: usb: f81604: handle short interrupt urb messages properly")
> Cc: stable@vger.kernel.org
> Reported-by: Dynetrex, Admin <admin@dynetrex.com>
> Closes: https://lore.kernel.org/all/A3834A07-5639-4779-844F-C5843DFC3928@dynetrex.com/
> Signed-off-by: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
I hope this patch goes into v7.3. While reviewing this patch the bot
found this issue:
Will the 2nd issue be fixed by
https://lore.kernel.org/all/20260901-f81604-fix-v1-1-c55b5178f9f8@fintek.com.tw/ ?
regards,
Marc
--- Patch [3]: [PATCH net 3/3] usb: f81604: fix struct f81604_int_data size mismatch ---
commit 6877926768a247e156dd43002e5a7320e9ac2d39
Author: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
usb: f81604: fix struct f81604_int_data size mismatch
This commit drops the __aligned(4) attribute from struct f81604_int_data.
With the attribute gone, sizeof() is 9 bytes, which matches what the
interrupt endpoint sends. With the old 12-byte padded size, the short URB
check in f81604_read_int_callback() threw away every valid interrupt
message, so TX completion was never processed.
Link: https://patch.msgid.link/20260824-f81604-fix-v2-1-fc9be5581394@fintek.com.tw
> 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;
Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra · Ranked High by: gpt-6-astra
This isn't a bug introduced by this patch, but there is a problem in the
short URB check that this patch adjusts. In f81604_read_int_callback(),
the check runs before the status switch:
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) {
case 0: /* success */
break;
case -ENOENT:
case -EPIPE:
case -EPROTO:
case -ESHUTDOWN:
return;
...
}
Suppose the URB completes with one of these error codes and actual_length
is 0. Won't it take the goto resubmit_urb path and never reach the
return?
On every ndo_stop, this call chain completes the interrupt URB with
-ENOENT and 0 bytes:
f81604_close()->f81604_unregister_urbs()->usb_kill_anchored_urbs()
The callback would then log a false "short int URB" warning. The
resubmit would fail with -EPERM because usb_kill_urb() has set
urb->reject, and that logs "failed to resubmit int urb".
On -EPIPE (endpoint stall) or -EPROTO, the URB would be resubmitted at
interval 1 instead of stopping. Each failing completion then logs
netdev_info and netdev_warn messages with no rate limit.
This ordering came from 7299b1b39a25, the commit this patch fixes. This
patch doesn't change the behaviour, since 0 < 9 just as 0 < 12.
Could the length check be applied only when urb->status is 0?
Discovered by: gpt-6-astra · Missed by: opus-5-5, sashiko-gemini · Confirmed by: opus-5-5 · Ranked Critical by: gpt-6-astra
This is a pre-existing issue, but this patch makes it reachable again.
f81604_close() cancels clear_reg_work before it kills the interrupt URBs
that can queue that work:
drivers/net/can/usb/f81604.c:f81604_close() {
...
netif_stop_queue(netdev);
cancel_work_sync(&priv->clear_reg_work);
close_candev(netdev);
f81604_unregister_urbs(priv);
...
}
What if an interrupt URB completes after cancel_work_sync() returns but
before usb_kill_anchored_urbs() runs? Can f81604_handle_can_bus_errors()
then queue the work again after a bus error, arbitration loss or overrun?
drivers/net/can/usb/f81604.c:f81604_handle_can_bus_errors() {
...
if (priv->clear_flags)
schedule_work(&priv->clear_reg_work);
...
}
On unbind or disconnect, f81604_disconnect() does this:
unregister_netdev(priv->netdev[i]);
free_candev(priv->netdev[i]);
Nothing cancels or flushes clear_reg_work again before free_candev().
f81604_clear_reg_work() gets priv via container_of() and then calls
f81604_sja1000_read() and f81604_sja1000_write() on it.
Would this be a use-after-free of the netdev private data and of the
embedded work_struct?
This ordering has been there since the driver was added in 88da17436973.
After 7299b1b39a25, every 9-byte interrupt failed the < 12 check, so this
schedule_work() call was effectively never reached. With this patch,
valid interrupts are handled again.
Could f81604_close() kill the anchored URBs before cancel_work_sync()?
Alternatively, could f81604_disconnect() cancel the work again before
free_candev()?
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread