* [PATCH v6 01/13] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 02/13] can: peak_usb: add missing includes Stéphane Grosjean via B4 Relay
` (11 subsequent siblings)
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel, Stefan Günther, stable
From: Stefan Günther <stefangun@pm.me>
When the device reports error counters, the driver incorrectly uses an
assignment (=) instead of a bitwise OR (|=) for CAN_ERR_CNT. This wipes
out the CAN_ERR_FLAG and CAN_ERR_CRTL flags, causing the error frame to
be sent to userspace as a regular CAN frame with ID 0x200.
Fix this by using a bitwise OR to preserve the flags.
Signed-off-by: Stefan Günther <stefangun@pm.me>
Cc: stable@vger.kernel.org
Fixes: 3e5c291c7942 ("can: add CAN_ERR_CNT flag to notify availability of error counter")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 8fd058c32856..785224e00c4e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -526,7 +526,7 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
/* Supply TX/RX error counters in case of
* controller error.
*/
- cf->can_id = CAN_ERR_CNT;
+ cf->can_id |= CAN_ERR_CNT;
cf->data[6] = mc->pdev->bec.txerr;
cf->data[7] = mc->pdev->bec.rxerr;
}
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 02/13] can: peak_usb: add missing includes
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 01/13] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 03/13] can: peak_usb: sort ctrlmode_supported flags Stéphane Grosjean via B4 Relay
` (10 subsequent siblings)
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Marc Kleine-Budde <mkl@pengutronix.de>
To make the header self contained, so that LSP servers don't throw errors,
make the header file self contained and include all needed header files.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 65999f04f4b7..9ad7d31d2720 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -11,6 +11,12 @@
#ifndef PCAN_USB_CORE_H
#define PCAN_USB_CORE_H
+#include <linux/can/dev.h>
+#include <linux/can/netlink.h>
+#include <linux/skbuff.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
/* PEAK-System vendor id. */
#define PCAN_USB_VENDOR_ID 0x0c72
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 03/13] can: peak_usb: sort ctrlmode_supported flags
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 01/13] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 02/13] can: peak_usb: add missing includes Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-24 3:58 ` netdev-bot+sashiko
2026-10-22 14:27 ` [PATCH v6 04/13] can: peak_usb: harden PCAN-USB message validation Stéphane Grosjean via B4 Relay
` (9 subsequent siblings)
12 siblings, 1 reply; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
Order the CAN_CTRLMODE_* flags in ctrlmode_supported according to
their numerical values.
This is a cosmetic change only and does not alter the driver
behavior.
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 3 ++-
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 32 ++++++++++++++++++-----------
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 3 ++-
3 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 785224e00c4e..169c00d93463 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -1016,7 +1016,8 @@ const struct peak_usb_adapter pcan_usb = {
.name = "PCAN-USB",
.device_id = PCAN_USB_PRODUCT_ID,
.ctrl_count = 1,
- .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
+ .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_3_SAMPLES |
CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_USB_CRYSTAL_HZ / 2,
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 0d46f4ce5dca..82502594a409 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -1198,9 +1198,11 @@ const struct peak_usb_adapter pcan_usb_fd = {
.name = "PCAN-USB FD",
.device_id = PCAN_USBFD_PRODUCT_ID,
.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
- .ctrlmode_supported = CAN_CTRLMODE_FD |
- CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+ .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_ONE_SHOT |
+ CAN_CTRLMODE_FD |
+ CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
@@ -1274,9 +1276,11 @@ const struct peak_usb_adapter pcan_usb_chip = {
.name = "PCAN-Chip USB",
.device_id = PCAN_USBCHIP_PRODUCT_ID,
.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
- .ctrlmode_supported = CAN_CTRLMODE_FD |
- CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+ .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_ONE_SHOT |
+ CAN_CTRLMODE_FD |
+ CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
@@ -1350,9 +1354,11 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
.name = "PCAN-USB Pro FD",
.device_id = PCAN_USBPROFD_PRODUCT_ID,
.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
- .ctrlmode_supported = CAN_CTRLMODE_FD |
- CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+ .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_ONE_SHOT |
+ CAN_CTRLMODE_FD |
+ CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
@@ -1426,9 +1432,11 @@ const struct peak_usb_adapter pcan_usb_x6 = {
.name = "PCAN-USB X6",
.device_id = PCAN_USBX6_PRODUCT_ID,
.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
- .ctrlmode_supported = CAN_CTRLMODE_FD |
- CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
+ .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_ONE_SHOT |
+ CAN_CTRLMODE_FD |
+ CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index b6be8c19e537..4c3d89a160af 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -1072,7 +1072,8 @@ const struct peak_usb_adapter pcan_usb_pro = {
.name = "PCAN-USB Pro",
.device_id = PCAN_USBPRO_PRODUCT_ID,
.ctrl_count = PCAN_USBPRO_CHANNEL_COUNT,
- .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
+ .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_3_SAMPLES |
CAN_CTRLMODE_ONE_SHOT,
.clock = {
.freq = PCAN_USBPRO_CRYSTAL_HZ,
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v6 03/13] can: peak_usb: sort ctrlmode_supported flags
2026-10-22 14:27 ` [PATCH v6 03/13] can: peak_usb: sort ctrlmode_supported flags Stéphane Grosjean via B4 Relay
@ 2026-10-24 3:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 19+ messages in thread
From: netdev-bot+sashiko @ 2026-10-24 3:58 UTC (permalink / raw)
To: s.grosjean
Cc: mkl, mailhol, wg, error27, lukas.magel, linux-can, linux-kernel,
stable, kuba
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message says the patch orders the CAN_CTRLMODE_* flags
in ctrlmode_supported by numerical value.
--
can: peak_usb: sort ctrlmode_supported flags
This commit reorders the CAN_CTRLMODE_* flags in the static
ctrlmode_supported initializers of the peak_usb adapters so that they
follow their numerical values. It is described as a cosmetic change with
no behavior change.
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 785224e00c4e9..169c00d93463c 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -1016,7 +1016,8 @@ const struct peak_usb_adapter pcan_usb = {
> .name = "PCAN-USB",
> .device_id = PCAN_USB_PRODUCT_ID,
> .ctrl_count = 1,
> - .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
> + .ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
> + CAN_CTRLMODE_3_SAMPLES |
> CAN_CTRLMODE_CC_LEN8_DLC,
[Severity: Low]
This isn't a bug, but the runtime ctrlmode_supported update in
pcan_usb_init() in the same file was not reordered:
drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_init() {
...
if (dev->device_rev >= 41) {
struct can_priv *priv = netdev_priv(dev->netdev);
priv->ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT |
CAN_CTRLMODE_LOOPBACK;
...
}
CAN_CTRLMODE_LOOPBACK is 0x01 and CAN_CTRLMODE_ONE_SHOT is 0x08, so by
the ordering described in the commit message LOOPBACK would come first.
The commit message says:
"Order the CAN_CTRLMODE_* flags in ctrlmode_supported according to
their numerical values."
It doesn't limit this to the static initializers. Should this line be
reordered too, for consistency? No later patch in the series appears to
change it.
As the commit message says, the resulting bitmask is identical either
way.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-canfd_check_channel_idx-v6-0-197704231551%40peak-system.fr
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 04/13] can: peak_usb: harden PCAN-USB message validation
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (2 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 03/13] can: peak_usb: sort ctrlmode_supported flags Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 05/13] can: peak_usb: replace open-coded bitmask operations with bitops Stéphane Grosjean via B4 Relay
` (8 subsequent siblings)
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel, stable
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
Add boundary checks when parsing PCAN-USB messages and reject
malformed buffers whose contents would otherwise lead to accesses
outside the received USB data area.
This prevents potential out-of-bounds memory accesses caused by
corrupted or malicious device messages.
Fixes: 46be265d3388 ("can: usb: PEAK-System Technik PCAN-USB specific part")
Cc: stable@vger.kernel.org
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 169c00d93463..5da91d5dc67a 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -558,6 +558,8 @@ static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
switch (ir) {
case PCAN_USB_ERR_CNT_DEC:
case PCAN_USB_ERR_CNT_INC:
+ if ((mc->ptr + 3) > mc->end)
+ return -EINVAL;
/* save rx/tx error counters from in the device context */
pdev->bec.rxerr = mc->ptr[1];
@@ -696,8 +698,11 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
mc->ptr += rec_len;
/* Ignore next byte (client private id) if SRR bit is set */
- if (can_id_flags & PCAN_USB_TX_SRR)
+ if (can_id_flags & PCAN_USB_TX_SRR) {
+ if ((mc->ptr + 1) > mc->end)
+ goto decode_failed;
mc->ptr++;
+ }
/* update statistics */
mc->netdev->stats.rx_bytes += cf->len;
@@ -733,14 +738,19 @@ static int pcan_usb_decode_msg(struct peak_usb_device *dev, u8 *ibuf, u32 lbuf)
int err;
for (err = 0; mc.rec_idx < mc.rec_cnt && !err; mc.rec_idx++) {
- u8 sl = *mc.ptr++;
-
- /* handle status and error frames here */
- if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
- err = pcan_usb_decode_status(&mc, sl);
- /* handle normal can frames here */
+ /* check if status_len byte can be read next */
+ if (mc.ptr >= mc.end) {
+ err = -EINVAL;
} else {
- err = pcan_usb_decode_data(&mc, sl);
+ u8 sl = *mc.ptr++;
+
+ /* handle status and error frames here */
+ if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
+ err = pcan_usb_decode_status(&mc, sl);
+ /* handle normal can frames here */
+ } else {
+ err = pcan_usb_decode_data(&mc, sl);
+ }
}
}
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 05/13] can: peak_usb: replace open-coded bitmask operations with bitops
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (3 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 04/13] can: peak_usb: harden PCAN-USB message validation Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 06/13] can: peak_usb: harden channel handling in PCAN-USB FD Stéphane Grosjean via B4 Relay
` (7 subsequent siblings)
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
The PCAN_USB_STATE_* flags of struct peak_usb_device are set, cleared
and tested from different contexts (ndo_open/ndo_stop, USB disconnect,
URB completion) with non-atomic open-coded bitmask operations.
Turn these flags into bit numbers, make the state field an unsigned
long and use set_bit(), clear_bit() and test_bit() instead, so that
updating one flag can no longer overwrite a concurrent update of
another one. The next patch relies on this to clear CONNECTED while a
channel may be opened.
dev->state is zeroed by alloc_candev(), so setting CONNECTED with
set_bit() at creation is equivalent to the former assignment.
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 4 ++--
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 10 +++++-----
drivers/net/can/usb/peak_usb/pcan_usb_core.h | 9 ++++++---
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 +-
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 6 +++---
5 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 5da91d5dc67a..93da0fd4f10e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -158,7 +158,7 @@ static int pcan_usb_send_cmd(struct peak_usb_device *dev, u8 f, u8 n, u8 *p)
int actual_length;
/* usb device unregistered? */
- if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
dev->cmd_buf[PCAN_USB_CMD_FUNC] = f;
@@ -188,7 +188,7 @@ static int pcan_usb_wait_rsp(struct peak_usb_device *dev, u8 f, u8 n, u8 *p)
int actual_length;
/* usb device unregistered? */
- if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
/* first, send command */
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 55aad01cd8ca..a1b17e1e6fea 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -263,7 +263,7 @@ static void peak_usb_read_bulk_callback(struct urb *urb)
/* protect from any incoming empty msgs */
if ((urb->actual_length > 0) && (dev->adapter->dev_decode_buf)) {
/* handle these kinds of msgs only if _start callback called */
- if (dev->state & PCAN_USB_STATE_STARTED) {
+ if (test_bit(PCAN_USB_STATE_STARTED, &dev->state)) {
err = dev->adapter->dev_decode_buf(dev, urb);
if (err)
pcan_dump_mem("received usb message",
@@ -537,7 +537,7 @@ static int peak_usb_start(struct peak_usb_device *dev)
goto err_adapter;
}
- dev->state |= PCAN_USB_STATE_STARTED;
+ set_bit(PCAN_USB_STATE_STARTED, &dev->state);
/* can set bus on now */
if (dev->adapter->dev_set_bus) {
@@ -631,7 +631,7 @@ static int peak_usb_ndo_stop(struct net_device *netdev)
{
struct peak_usb_device *dev = netdev_priv(netdev);
- dev->state &= ~PCAN_USB_STATE_STARTED;
+ clear_bit(PCAN_USB_STATE_STARTED, &dev->state);
netif_stop_queue(netdev);
close_candev(netdev);
@@ -941,7 +941,7 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
dev->netdev = netdev;
dev->adapter = peak_usb_adapter;
dev->ctrl_idx = ctrl_idx;
- dev->state = PCAN_USB_STATE_CONNECTED;
+ set_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
dev->ep_msg_in = peak_usb_adapter->ep_msg_in;
dev->ep_msg_out = peak_usb_adapter->ep_msg_out[ctrl_idx];
@@ -1044,7 +1044,7 @@ static void peak_usb_disconnect(struct usb_interface *intf)
char name[IFNAMSIZ];
dev_prev_siblings = dev->prev_siblings;
- dev->state &= ~PCAN_USB_STATE_CONNECTED;
+ clear_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
strscpy(name, netdev->name, IFNAMSIZ);
unregister_candev(netdev);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 9ad7d31d2720..cb8777c3e08e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -109,15 +109,18 @@ struct peak_tx_urb_context {
struct urb *urb;
};
-#define PCAN_USB_STATE_CONNECTED 0x00000001
-#define PCAN_USB_STATE_STARTED 0x00000002
+/* bit numbers for struct peak_usb_device::state (used with set_bit()/
+ * clear_bit()/test_bit() for atomic read-modify-write across contexts)
+ */
+#define PCAN_USB_STATE_CONNECTED 0
+#define PCAN_USB_STATE_STARTED 1
/* PEAK-System USB device */
struct peak_usb_device {
struct can_priv can;
const struct peak_usb_adapter *adapter;
unsigned int ctrl_idx;
- u32 state;
+ unsigned long state;
struct usb_device *udev;
struct net_device *netdev;
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 82502594a409..15d275d6a127 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -202,7 +202,7 @@ static int pcan_usb_fd_send_cmd(struct peak_usb_device *dev, void *cmd_tail)
ptrdiff_t cmd_len;
/* usb device unregistered? */
- if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
/* if a packet is not filled completely by commands, the command list
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 4c3d89a160af..cbc23953ce61 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -210,7 +210,7 @@ static int pcan_usb_pro_send_cmd(struct peak_usb_device *dev,
int err;
/* usb device unregistered? */
- if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
err = usb_bulk_msg(dev->udev,
@@ -234,7 +234,7 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
int i, err = 0;
/* usb device unregistered? */
- if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
req_data_type = pum->u.rec_buffer[4];
@@ -318,7 +318,7 @@ int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
unsigned int p;
/* usb device unregistered? */
- if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
req_type = USB_TYPE_VENDOR | USB_RECIP_OTHER;
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 06/13] can: peak_usb: harden channel handling in PCAN-USB FD
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (4 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 05/13] can: peak_usb: replace open-coded bitmask operations with bitops Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-24 3:58 ` netdev-bot+sashiko
2026-10-22 14:27 ` [PATCH v6 07/13] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR Stéphane Grosjean via B4 Relay
` (6 subsequent siblings)
12 siblings, 1 reply; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
Records received from PCAN-USB FD family devices carry a CAN channel
number used to index the adapter CAN device table usb_if->dev[].
The decoders only checked this number against the size of the table
(2 entries). On single-channel adapters (PCAN-USB FD, PCAN-Chip USB),
dev[1] is never set, so a record for channel 1 led to a NULL pointer
dereference in URB completion context. On two-channel adapters, a
channel slot is also NULL until that channel has been initialized,
while the RX URBs of channel 0 may already be running.
Check the channel number against the number of controllers of the
adapter and reject records for a channel whose slot is empty. Such a
record makes the decoder stop processing the rest of the URB.
When probing a channel fails after its slot has been filled, the
channel is freed but its slot kept pointing to it. Clear the slot in
pcan_usb_fd_free() and, before freeing the channel, stop RX on the
sibling channels already created, since the whole interface is then
released anyway. Slots are published with smp_store_release() and
read with READ_ONCE().
Finally, peak_usb_disconnect() now stops RX on all channels before
unregistering any of them: it clears CONNECTED, which makes
peak_usb_ndo_open() fail, then poisons the RX anchors. This matters
when it is called from the probe error path, where the USB core has
not disabled the interface.
CONNECTED is cleared under RTNL, to serialize with peak_usb_ndo_open():
otherwise, an ndo_open() that has already passed the CONNECTED test
may have anchored an RX URB without having submitted it yet, which
makes usb_poison_anchored_urbs() busy-wait until it is submitted.
Fixes: a6921dd524fe ("can: peak_usb: add range checking in decode operations")
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 38 ++++++++++++++++-
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 62 +++++++++++++++++++++-------
2 files changed, 83 insertions(+), 17 deletions(-)
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 a1b17e1e6fea..49d2c01b8ef7 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -13,6 +13,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
#include <linux/signal.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
@@ -574,6 +575,9 @@ static int peak_usb_ndo_open(struct net_device *netdev)
struct peak_usb_device *dev = netdev_priv(netdev);
int err;
+ if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
+ return -ENODEV;
+
/* common open */
err = open_candev(netdev);
if (err)
@@ -904,6 +908,29 @@ int pcan_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info
return 0;
}
+/*
+ * stop RX on dev and on all its previous siblings: clearing CONNECTED makes
+ * peak_usb_ndo_open() fail, then poisoning the RX anchors waits for running
+ * completion handlers and makes any later submission on them fail.
+ *
+ * peak_usb_ndo_open() runs under RTNL: clearing CONNECTED under RTNL ensures
+ * that no ndo_open() is in progress (so no RX URB is anchored but not yet
+ * submitted, which usb_poison_anchored_urbs() would busy-wait on) and that
+ * any later one fails.
+ */
+static void peak_usb_stop_rx(struct peak_usb_device *dev)
+{
+ struct peak_usb_device *d;
+
+ rtnl_lock();
+ for (d = dev; d; d = d->prev_siblings)
+ clear_bit(PCAN_USB_STATE_CONNECTED, &d->state);
+ rtnl_unlock();
+
+ for (d = dev; d; d = d->prev_siblings)
+ usb_poison_anchored_urbs(&d->rx_submitted);
+}
+
/*
* create one device which is attached to CAN controller #ctrl_idx of the
* usb adapter.
@@ -1014,6 +1041,12 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
return 0;
adap_dev_free:
+ /* the whole interface is released when probing a channel fails:
+ * stop RX on the channels already created, since their RX URBs may
+ * carry records for this one.
+ */
+ peak_usb_stop_rx(dev->prev_siblings);
+
if (dev->adapter->dev_free)
dev->adapter->dev_free(dev);
@@ -1038,13 +1071,14 @@ static void peak_usb_disconnect(struct usb_interface *intf)
struct peak_usb_device *dev;
struct peak_usb_device *dev_prev_siblings;
- /* unregister as many netdev devices as siblings */
+ /* stop RX on all channels before unregistering and freeing them */
+ peak_usb_stop_rx(usb_get_intfdata(intf));
+
for (dev = usb_get_intfdata(intf); dev; dev = dev_prev_siblings) {
struct net_device *netdev = dev->netdev;
char name[IFNAMSIZ];
dev_prev_siblings = dev->prev_siblings;
- clear_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
strscpy(name, netdev->name, IFNAMSIZ);
unregister_candev(netdev);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 15d275d6a127..eee5b5eaa247 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -60,6 +60,7 @@ struct __packed pcan_ufd_fw_info {
/* handle device specific info used by the netdevices */
struct pcan_usb_fd_if {
+ const struct peak_usb_adapter *adapter;
struct peak_usb_device *dev[PCAN_USB_MAX_CHANNEL];
struct pcan_ufd_fw_info fw_info;
struct peak_time_ref time_ref;
@@ -536,10 +537,16 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
struct sk_buff *skb;
const u16 rx_msg_flags = le16_to_cpu(rm->flags);
- if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
- return -ENOMEM;
+ /* Reject invalid channel numbers reported by the firmware */
+ if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
+ return -EINVAL;
+
+ dev = READ_ONCE(usb_if->dev[pucan_msg_get_channel(rm)]);
+
+ /* reject records for a channel that is not (or no longer) set up */
+ if (!dev)
+ return -EINVAL;
- dev = usb_if->dev[pucan_msg_get_channel(rm)];
netdev = dev->netdev;
if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {
@@ -605,10 +612,16 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
struct can_frame *cf;
struct sk_buff *skb;
- if (pucan_stmsg_get_channel(sm) >= ARRAY_SIZE(usb_if->dev))
- return -ENOMEM;
+ /* Reject invalid channel numbers reported by the firmware */
+ if (pucan_stmsg_get_channel(sm) >= usb_if->adapter->ctrl_count)
+ return -EINVAL;
+
+ dev = READ_ONCE(usb_if->dev[pucan_stmsg_get_channel(sm)]);
+
+ /* reject records for a channel that is not (or no longer) set up */
+ if (!dev)
+ return -EINVAL;
- dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
pdev = container_of(dev, struct pcan_usb_fd_device, dev);
netdev = dev->netdev;
@@ -662,10 +675,16 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
struct pcan_usb_fd_device *pdev;
struct peak_usb_device *dev;
- if (pucan_ermsg_get_channel(er) >= ARRAY_SIZE(usb_if->dev))
+ /* Reject invalid channel numbers reported by the firmware */
+ if (pucan_ermsg_get_channel(er) >= usb_if->adapter->ctrl_count)
+ return -EINVAL;
+
+ dev = READ_ONCE(usb_if->dev[pucan_ermsg_get_channel(er)]);
+
+ /* reject records for a channel that is not (or no longer) set up */
+ if (!dev)
return -EINVAL;
- dev = usb_if->dev[pucan_ermsg_get_channel(er)];
pdev = container_of(dev, struct pcan_usb_fd_device, dev);
/* keep a trace of tx and rx error counters for later use */
@@ -685,10 +704,16 @@ static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
struct can_frame *cf;
struct sk_buff *skb;
- if (pufd_omsg_get_channel(ov) >= ARRAY_SIZE(usb_if->dev))
+ /* Reject invalid channel numbers reported by the firmware */
+ if (pufd_omsg_get_channel(ov) >= usb_if->adapter->ctrl_count)
+ return -EINVAL;
+
+ dev = READ_ONCE(usb_if->dev[pufd_omsg_get_channel(ov)]);
+
+ /* reject records for a channel that is not (or no longer) set up */
+ if (!dev)
return -EINVAL;
- dev = usb_if->dev[pufd_omsg_get_channel(ov)];
netdev = dev->netdev;
/* allocate an skb to store the error frame */
@@ -981,6 +1006,9 @@ static int pcan_usb_fd_init(struct peak_usb_device *dev)
if (!pdev->usb_if)
goto err_out;
+ /* needed by the decoders to check channel numbers */
+ pdev->usb_if->adapter = dev->adapter;
+
/* allocate command buffer once for all for the interface */
pdev->cmd_buffer_addr = kzalloc(PCAN_UFD_CMD_BUFFER_SIZE,
GFP_KERNEL);
@@ -1054,7 +1082,8 @@ static int pcan_usb_fd_init(struct peak_usb_device *dev)
fw_info = &pdev->usb_if->fw_info;
}
- pdev->usb_if->dev[dev->ctrl_idx] = dev;
+ /* publish dev to the decoders, after the ctrlmode copies above */
+ smp_store_release(&pdev->usb_if->dev[dev->ctrl_idx], dev);
dev->can_channel_id =
le32_to_cpu(pdev->usb_if->fw_info.dev_id[dev->ctrl_idx]);
@@ -1124,14 +1153,17 @@ static void pcan_usb_fd_exit(struct peak_usb_device *dev)
}
}
-/* called when the USB adapter is unplugged */
+/* called when the USB adapter is unplugged, or when probing a sibling fails */
static void pcan_usb_fd_free(struct peak_usb_device *dev)
{
+ struct pcan_usb_fd_device *pdev =
+ container_of(dev, struct pcan_usb_fd_device, dev);
+
+ /* clear this channel's slot: dev is going to be freed */
+ WRITE_ONCE(pdev->usb_if->dev[dev->ctrl_idx], NULL);
+
/* last device: can free shared objects now */
if (!dev->prev_siblings && !dev->next_siblings) {
- struct pcan_usb_fd_device *pdev =
- container_of(dev, struct pcan_usb_fd_device, dev);
-
/* free commands buffer */
kfree(pdev->cmd_buffer_addr);
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v6 06/13] can: peak_usb: harden channel handling in PCAN-USB FD
2026-10-22 14:27 ` [PATCH v6 06/13] can: peak_usb: harden channel handling in PCAN-USB FD Stéphane Grosjean via B4 Relay
@ 2026-10-24 3:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 19+ messages in thread
From: netdev-bot+sashiko @ 2026-10-24 3:58 UTC (permalink / raw)
To: s.grosjean
Cc: mkl, mailhol, wg, error27, lukas.magel, linux-can, linux-kernel,
stable, kuba
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (2) · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The probe-error cleanup in peak_usb_create_dev() (adap_dev_free
label) calls peak_usb_stop_rx(dev->prev_siblings).
Pre-existing issues:
- [Medium] PCAN-USB Pro has the same NULL channel-slot bug that this
patch fixes for PCAN-USB FD.
- [Medium] When peak_usb_disconnect() runs from the probe-error path,
peak_usb_stop_rx() clears PCAN_USB_STATE_CONNECTED on all siblings…
--
can: peak_usb: harden channel handling in PCAN-USB FD
The PCAN-USB FD decoders now check the channel number against the
adapter's controller count, and they drop records for a channel whose
usb_if->dev[] slot is NULL. The slot is cleared in pcan_usb_fd_free().
A new helper, peak_usb_stop_rx(), clears CONNECTED under RTNL and poisons
the RX anchors. It is called from the probe error path and from
peak_usb_disconnect().
> 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 a1b17e1e6fea6..49d2c01b8ef7e 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[ ... ]
> @@ -1014,6 +1041,12 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
> return 0;
>
> adap_dev_free:
> + /* the whole interface is released when probing a channel fails:
> + * stop RX on the channels already created, since their RX URBs may
> + * carry records for this one.
> + */
> + peak_usb_stop_rx(dev->prev_siblings);
[Severity: High]
Should this also stop RX on dev, and not only on its previous siblings?
The comment above peak_usb_stop_rx() says it stops "dev and on all its
previous siblings", but this call skips the channel that is failing.
At this commit, peak_usb_create_dev() calls register_candev() before
dev_init() and dev_set_bus(). So userspace may already have opened the
failing channel, and its RX URBs may be in flight, when
dev_set_bus(dev, 0) fails.
On a single-channel PCAN-USB FD, dev->prev_siblings is NULL, so nothing
gets stopped. The error path then runs:
peak_usb_create_dev()
dev->adapter->dev_free(dev)
pcan_usb_fd_free()
kfree(pdev->cmd_buffer_addr);
kfree(pdev->usb_if);
unregister_candev(netdev)
peak_usb_ndo_stop()
dev->adapter->dev_stop(dev)
pcan_usb_fd_stop()
pdev->usb_if->dev_opened_count--;
Can the RX completions of this channel, and then pcan_usb_fd_stop(), use
usb_if and the command buffer after they have been freed here?
The later patch in this series, "can: peak_usb: register the netdevs only
once all channels are set up", seems to close this window. It moves
register_candev() into peak_usb_probe(), after all channels have been
created. At this commit, though, the window is still open.
> if (dev->adapter->dev_free)
> dev->adapter->dev_free(dev);
>
[ ... ]
> @@ -1038,13 +1071,14 @@ static void peak_usb_disconnect(struct usb_interface *intf)
> struct peak_usb_device *dev;
> struct peak_usb_device *dev_prev_siblings;
>
> - /* unregister as many netdev devices as siblings */
> + /* stop RX on all channels before unregistering and freeing them */
> + peak_usb_stop_rx(usb_get_intfdata(intf));
> +
> for (dev = usb_get_intfdata(intf); dev; dev = dev_prev_siblings) {
> struct net_device *netdev = dev->netdev;
> char name[IFNAMSIZ];
>
> dev_prev_siblings = dev->prev_siblings;
> - clear_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
> strscpy(name, netdev->name, IFNAMSIZ);
>
> unregister_candev(netdev);
[Severity: Medium]
This is a pre-existing issue, but when peak_usb_disconnect() is called
from the probe error path, the device is still attached and its interface
has not been disabled.
peak_usb_stop_rx() clears PCAN_USB_STATE_CONNECTED on every channel before
any unregister_candev() runs. For a channel that is already up,
unregister_candev() then calls:
peak_usb_ndo_stop()
dev->adapter->dev_stop(dev)
dev->adapter->dev_set_bus(dev, 0)
pcan_usb_fd_send_cmd()
and pcan_usb_fd_send_cmd() starts with:
/* usb device unregistered? */
if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
After a failed probe, is the CAN controller left bus-on, while
peak_usb_ndo_stop() treats the bus-off command as successful?
The old loop already cleared CONNECTED just before each
unregister_candev(), so these commands were dropped before this patch
too. The patch now clears it on all channels at once.
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index 15d275d6a1277..eee5b5eaa247d 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
[ ... ]
> @@ -536,10 +537,16 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
> struct sk_buff *skb;
> const u16 rx_msg_flags = le16_to_cpu(rm->flags);
>
> - if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
> - return -ENOMEM;
> + /* Reject invalid channel numbers reported by the firmware */
> + if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
> + return -EINVAL;
> +
> + dev = READ_ONCE(usb_if->dev[pucan_msg_get_channel(rm)]);
> +
> + /* reject records for a channel that is not (or no longer) set up */
> + if (!dev)
> + return -EINVAL;
>
> - dev = usb_if->dev[pucan_msg_get_channel(rm)];
> netdev = dev->netdev;
[Severity: Medium]
This isn't a bug introduced by this patch, but the PCAN-USB Pro decoders
look like they have the same problem that this patch fixes for PCAN-USB
FD.
In pcan_usb_pro.c, pcan_usb_pro_handle_canmsg() and
pcan_usb_pro_handle_error() check ctrl_idx only against the table size,
then dereference the slot:
if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
return -EINVAL;
dev = usb_if->dev[ctrl_idx];
netdev = dev->netdev;
The channels share the bulk-in endpoint, so channel 0's RX URBs could
carry a channel 1 record before pcan_usb_pro_init() has filled
usb_if->dev[1]. Can that lead to a NULL pointer dereference in URB
completion context? At this commit, register_candev() runs before
dev_init() in peak_usb_create_dev(), so channel 0 can already be open at
that point.
Unlike the new pcan_usb_fd_free(), pcan_usb_pro_free() also leaves
usb_if->dev[dev->ctrl_idx] set when the channel is freed.
The commit message does not mention PCAN-USB Pro. Two later patches in
this series appear to fix this: "can: peak_usb: reject records for unset
channels in PCAN-USB Pro" and "can: peak_usb: register the netdevs only
once all channels are set up".
>
> if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-canfd_check_channel_idx-v6-0-197704231551%40peak-system.fr
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 07/13] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (5 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 06/13] can: peak_usb: harden channel handling in PCAN-USB FD Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 08/13] can: peak_usb: reject records for unset channels in PCAN-USB Pro Stéphane Grosjean via B4 Relay
` (5 subsequent siblings)
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel, stable
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
pcan_usb_encode_msg() appends a one-byte "writer id" whenever
PCAN_USB_TX_SRR is set in can_id_flags, for both RTR and non-RTR
frames: the data block and the SRR writer id byte are two independent,
unconditional steps.
pcan_usb_decode_data() only consumed that extra byte in the non-RTR
branch, inside the same conditional block that reads the CAN data
payload. A self-received RTR frame with the SRR bit set (e.g. a
loopback echo with CAN_CTRLMODE_LOOPBACK) therefore left the RX cursor
one byte short, causing the private id byte to be misparsed as the
next record's status/len byte and the rest of the URB to be misparsed
or rejected.
Move the private id byte skip out of the RTR/non-RTR branch so it is
applied whenever SRR is set, matching the encoder's layout.
Since a frame can now be dropped when this byte is missing, update
rx_bytes together with rx_packets, once the frame has been fully
decoded.
Fixes: 3a7939495ce8 ("can: peak_usb: PCAN-USB: add support of loopback and one-shot mode")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 93da0fd4f10e..6a5ecb10c4f3 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -655,6 +655,7 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
struct sk_buff *skb;
struct can_frame *cf;
struct skb_shared_hwtstamps *hwts;
+ u8 rx_bytes = 0;
u32 can_id_flags;
skb = alloc_can_skb(mc->netdev, &cf);
@@ -696,18 +697,21 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
memcpy(cf->data, mc->ptr, cf->len);
mc->ptr += rec_len;
+ rx_bytes = cf->len;
+ }
- /* Ignore next byte (client private id) if SRR bit is set */
- if (can_id_flags & PCAN_USB_TX_SRR) {
- if ((mc->ptr + 1) > mc->end)
- goto decode_failed;
- mc->ptr++;
- }
-
- /* update statistics */
- mc->netdev->stats.rx_bytes += cf->len;
+ /* Ignore next byte (client private id) if SRR bit is set.
+ * Note: this test must be done even when the frame is an RTR frame.
+ */
+ if (can_id_flags & PCAN_USB_TX_SRR) {
+ if ((mc->ptr + 1) > mc->end)
+ goto decode_failed;
+ mc->ptr++;
}
+
+ /* update statistics */
mc->netdev->stats.rx_packets++;
+ mc->netdev->stats.rx_bytes += rx_bytes;
/* convert timestamp into kernel time */
hwts = skb_hwtstamps(skb);
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 08/13] can: peak_usb: reject records for unset channels in PCAN-USB Pro
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (6 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 07/13] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-24 3:58 ` netdev-bot+sashiko
2026-10-22 14:27 ` [PATCH v6 09/13] can: peak_usb: register the netdevs only once all channels are set up Stéphane Grosjean via B4 Relay
` (4 subsequent siblings)
12 siblings, 1 reply; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
Records received from a PCAN-USB Pro device carry a CAN channel number
used to index the CAN device table usb_if->dev[]. The decoders only
check this number against the size of the table.
The slot of channel 1 stays NULL until that channel has been
initialized, while the RX URBs of channel 0, which share the same
bulk-in endpoint, may already be running. A record for channel 1
received in that window leads to a NULL pointer dereference in URB
completion context. Moreover, when probing channel 1 fails after its
slot has been filled, the slot keeps pointing to the freed channel.
Reject records for a channel whose slot is empty, and clear the slot
of a channel in pcan_usb_pro_free(). Slots are published with
smp_store_release() and read with READ_ONCE().
Fixes: d8a199355f8f ("can: usb: PEAK-System Technik PCAN-USB Pro specific part")
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index cbc23953ce61..151c5ea2859b 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -543,7 +543,12 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
return -EINVAL;
- dev = usb_if->dev[ctrl_idx];
+ dev = READ_ONCE(usb_if->dev[ctrl_idx]);
+
+ /* reject records for a channel that is not (or no longer) set up */
+ if (!dev)
+ return -EINVAL;
+
netdev = dev->netdev;
skb = alloc_can_skb(netdev, &can_frame);
@@ -590,7 +595,12 @@ static int pcan_usb_pro_handle_error(struct pcan_usb_pro_interface *usb_if,
if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
return -EINVAL;
- dev = usb_if->dev[ctrl_idx];
+ dev = READ_ONCE(usb_if->dev[ctrl_idx]);
+
+ /* reject records for a channel that is not (or no longer) set up */
+ if (!dev)
+ return -EINVAL;
+
netdev = dev->netdev;
/* nothing should be sent while in BUS_OFF state */
@@ -933,7 +943,8 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
}
pdev->usb_if = usb_if;
- usb_if->dev[dev->ctrl_idx] = dev;
+ /* publish dev to the decoders */
+ smp_store_release(&usb_if->dev[dev->ctrl_idx], dev);
/* set LED in default state (end of init phase) */
pcan_usb_pro_set_led(dev, PCAN_USBPRO_LED_DEVICE, 1);
@@ -981,6 +992,9 @@ static void pcan_usb_pro_exit(struct peak_usb_device *dev)
*/
static void pcan_usb_pro_free(struct peak_usb_device *dev)
{
+ /* clear this channel's slot: dev is going to be freed */
+ WRITE_ONCE(pcan_usb_pro_dev_if(dev)->dev[dev->ctrl_idx], NULL);
+
/* last device: can free pcan_usb_pro_interface object now */
if (!dev->prev_siblings && !dev->next_siblings)
kfree(pcan_usb_pro_dev_if(dev));
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v6 08/13] can: peak_usb: reject records for unset channels in PCAN-USB Pro
2026-10-22 14:27 ` [PATCH v6 08/13] can: peak_usb: reject records for unset channels in PCAN-USB Pro Stéphane Grosjean via B4 Relay
@ 2026-10-24 3:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 19+ messages in thread
From: netdev-bot+sashiko @ 2026-10-24 3:58 UTC (permalink / raw)
To: s.grosjean
Cc: mkl, mailhol, wg, error27, lukas.magel, linux-can, linux-kernel,
stable, kuba
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] In pcan_usb_pro_decode_buf(), a record for a channel whose
usb_if->dev[] slot is NULL now makes…
Pre-existing issues:
- [High] Use-after-free in the probe error path of peak_usb_create_dev().
- [High] NULL pointer dereference when a channel is opened before
dev_init().
--
can: peak_usb: reject records for unset channels in PCAN-USB Pro
This commit makes the PCAN-USB Pro record decoders reject a record when
the channel's usb_if->dev[] slot is still NULL. It also clears the slot in
pcan_usb_pro_free(). Slots are now published with smp_store_release() and
read with READ_ONCE().
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index cbc23953ce610..151c5ea2859be 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -543,7 +543,12 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
> if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
> return -EINVAL;
>
> - dev = usb_if->dev[ctrl_idx];
> + dev = READ_ONCE(usb_if->dev[ctrl_idx]);
> +
> + /* reject records for a channel that is not (or no longer) set up */
> + if (!dev)
> + return -EINVAL;
[Severity: Medium]
Should an empty slot be treated as an error here? The commit message
describes this as an expected window: channel 0's RX is running before
channel 1 has been initialized.
A negative return makes pcan_usb_pro_decode_buf() jump to fail and stop
parsing the URB:
pcan_usb_pro_decode_buf() {
...
case PCAN_USBPRO_RXRTR:
err = pcan_usb_pro_handle_canmsg(usb_if, &pr->rx_msg);
if (err < 0)
goto fail;
...
fail:
if (err)
pcan_dump_mem("received msg",
urb->transfer_buffer, urb->actual_length);
}
Both channels share the same bulk-in endpoint. Wouldn't that drop every
record that follows the channel 1 record in the same transfer? That
includes channel 0 CAN frames, channel 0 status records and RXTS
calibration records.
The non-zero return also makes peak_usb_read_bulk_callback() dump the
buffer a second time:
err = dev->adapter->dev_decode_buf(dev, urb);
if (err)
pcan_dump_mem("received usb message",
urb->transfer_buffer,
urb->transfer_buffer_length);
Neither dump is rate limited, and both run from URB completion context.
Would it work better to skip just this record and go on to the next one?
The same applies to the -EINVAL return added to
pcan_usb_pro_handle_error().
Two later commits in the series seem to resolve this.
"can: peak_usb: register the netdevs only once all channels are set up"
makes this branch unreachable while RX is running.
"can: peak_usb: dump undecodable URBs once and rate limit decode errors"
removes the dump in pcan_usb_pro_decode_buf() and rate limits the one in
the callback.
At this commit, though, the window is still there.
[ ... ]
> @@ -933,7 +943,8 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
> }
>
> pdev->usb_if = usb_if;
> - usb_if->dev[dev->ctrl_idx] = dev;
> + /* publish dev to the decoders */
> + smp_store_release(&usb_if->dev[dev->ctrl_idx], dev);
[Severity: High]
This is a pre-existing issue, but pdev->usb_if is only assigned here,
after the netdev has already been registered. In peak_usb_create_dev():
set_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
...
err = register_candev(netdev);
...
if (dev->adapter->dev_init) {
err = dev->adapter->dev_init(dev);
pcan_usb_pro_init() also issues several synchronous USB requests on
channel 0 before it gets to this assignment.
If canX is configured and opened in that window, can
peak_usb_ndo_open()->...->pcan_usb_pro_start() dereference a NULL usb_if?
if (pdev->usb_if->dev_opened_count == 0) {
RX completions in the same window would also reach
pcan_usb_pro_decode_buf() with a NULL usb_if. The handlers do
READ_ONCE(usb_if->dev[ctrl_idx]) before the new check, so the added NULL
check on dev does not cover this case.
The later commit "can: peak_usb: register the netdevs only once all
channels are set up" seems to fix this. It registers the netdevs in
peak_usb_probe() only after every channel's dev_init() and dev_set_bus()
have succeeded, and its commit message names this dereference.
> @@ -981,6 +992,9 @@ static void pcan_usb_pro_exit(struct peak_usb_device *dev)
> */
> static void pcan_usb_pro_free(struct peak_usb_device *dev)
> {
> + /* clear this channel's slot: dev is going to be freed */
> + WRITE_ONCE(pcan_usb_pro_dev_if(dev)->dev[dev->ctrl_idx], NULL);
> +
> /* last device: can free pcan_usb_pro_interface object now */
> if (!dev->prev_siblings && !dev->next_siblings)
> kfree(pcan_usb_pro_dev_if(dev));
[Severity: High]
This isn't a bug introduced by this patch, but can this kfree() cause a
use-after-free on the probe error path in peak_usb_create_dev()?
adap_dev_free:
peak_usb_stop_rx(dev->prev_siblings);
if (dev->adapter->dev_free)
dev->adapter->dev_free(dev);
lbl_unregister_candev:
unregister_candev(netdev);
For channel 0, dev->prev_siblings is NULL, so no RX gets stopped. Then
pcan_usb_pro_free() frees the shared pcan_usb_pro_interface before the
netdev is unregistered.
Suppose can0 was opened between register_candev() and a failing
dev_set_bus(dev, 0). unregister_candev()->...->pcan_usb_pro_stop() would
then read and decrement dev_opened_count in freed memory:
if (pdev->usb_if->dev_opened_count == 1)
pcan_usb_pro_set_ts(dev, 0);
pdev->usb_if->dev_opened_count--;
Channel 0's own RX URBs were never poisoned, so they could also complete
into pcan_usb_pro_decode_buf() with the freed usb_if.
The WRITE_ONCE() added here runs before the kfree(), so it is fine by
itself. The later commit "can: peak_usb: register the netdevs only once
all channels are set up" removes register_candev() from
peak_usb_create_dev(), and its commit message names this use-after-free.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-canfd_check_channel_idx-v6-0-197704231551%40peak-system.fr
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 09/13] can: peak_usb: register the netdevs only once all channels are set up
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (7 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 08/13] can: peak_usb: reject records for unset channels in PCAN-USB Pro Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-24 3:58 ` netdev-bot+sashiko
2026-10-22 14:27 ` [PATCH v6 10/13] can: peak_usb: validate the data length of PCAN-USB Pro CAN records Stéphane Grosjean via B4 Relay
` (3 subsequent siblings)
12 siblings, 1 reply; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
peak_usb_create_dev() registers the netdev of a channel before calling
dev_init(), dev_set_bus() and dev_get_can_channel_id(). Userspace can
therefore open or configure the netdev while the channel is still being
initialized, outside of rtnl_lock():
- peak_usb_ndo_open() may run before dev_init() has set up the adapter
private data, e.g. pdev->usb_if, which is then dereferenced while
still NULL by pcan_usb_pro_start() or pcan_usb_fd_start();
- the commands sent by the open or bittiming callbacks may race with
the ones sent by dev_init() on the same command buffer;
- dev_set_bus(dev, 0) may switch the controller off right after
peak_usb_ndo_open() has started it.
On multi-channel adapters, the same applies to the channels already
created while a sibling is being initialized, since they share the
interface resources: on PCAN-USB FD, the command buffer and the
firmware info used by the sibling init, and the ctrlmode copied from
the first channel.
Moreover, if dev_init() or dev_set_bus() fails, the adapter private
data is released before unregister_candev(), which closes the netdev
if it has been opened in the meantime, and then accesses the released
data (interface object and, on PCAN-USB FD, command buffer).
Register the netdevs in peak_usb_probe(), only once all the channels
of the interface have been initialized, and only unregister the
registered ones in peak_usb_disconnect(). Since no netdev can be opened
during probe anymore, there is no RX to stop when probing a channel
fails. Since the channel id is now read before the netdev is
registered, PCAN-USB reports a failure to read it with dev_err()
instead of netdev_err().
Fixes: bb4785551f64 ("can: usb: PEAK-System Technik USB adapters driver core")
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 7 +++-
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 61 ++++++++++++++++------------
drivers/net/can/usb/peak_usb/pcan_usb_core.h | 1 +
3 files changed, 42 insertions(+), 27 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 6a5ecb10c4f3..8a0a132983c4 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -390,9 +390,12 @@ static int pcan_usb_get_can_channel_id(struct peak_usb_device *dev, u32 *can_ch_
int err;
err = pcan_usb_wait_rsp(dev, PCAN_USB_CMD_DEVID, PCAN_USB_GET, args);
+ /* explicit use of dev_xxx() instead of netdev_xxx() here: this is
+ * also called during probe, before the netdev is registered.
+ */
if (err)
- netdev_err(dev->netdev, "getting can channel id failure: %d\n", err);
-
+ dev_err(dev->netdev->dev.parent,
+ "getting can channel id failure: %d\n", err);
else
*can_ch_id = args[0];
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 49d2c01b8ef7..ea6c28f3ed0b 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -1006,15 +1006,6 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
SET_NETDEV_DEV(netdev, &intf->dev);
netdev->dev_id = ctrl_idx;
- err = register_candev(netdev);
- if (err) {
- dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
- goto lbl_restore_intf_data;
- }
-
- if (dev->prev_siblings)
- (dev->prev_siblings)->next_siblings = dev;
-
/* keep hw revision into the netdevice */
tmp16 = le16_to_cpu(usb_dev->descriptor.bcdDevice);
dev->device_rev = tmp16 >> 8;
@@ -1022,7 +1013,7 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
if (dev->adapter->dev_init) {
err = dev->adapter->dev_init(dev);
if (err)
- goto lbl_unregister_candev;
+ goto lbl_restore_intf_data;
}
/* set bus off */
@@ -1035,24 +1026,15 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
/* get CAN channel id early */
dev->adapter->dev_get_can_channel_id(dev, &dev->can_channel_id);
- netdev_info(netdev, "attached to %s channel %u (device 0x%08X)\n",
- peak_usb_adapter->name, ctrl_idx, dev->can_channel_id);
+ if (dev->prev_siblings)
+ (dev->prev_siblings)->next_siblings = dev;
return 0;
adap_dev_free:
- /* the whole interface is released when probing a channel fails:
- * stop RX on the channels already created, since their RX URBs may
- * carry records for this one.
- */
- peak_usb_stop_rx(dev->prev_siblings);
-
if (dev->adapter->dev_free)
dev->adapter->dev_free(dev);
-lbl_unregister_candev:
- unregister_candev(netdev);
-
lbl_restore_intf_data:
usb_set_intfdata(intf, dev->prev_siblings);
kfree(dev->cmd_buf);
@@ -1081,7 +1063,8 @@ static void peak_usb_disconnect(struct usb_interface *intf)
dev_prev_siblings = dev->prev_siblings;
strscpy(name, netdev->name, IFNAMSIZ);
- unregister_candev(netdev);
+ if (test_bit(PCAN_USB_STATE_REGISTERED, &dev->state))
+ unregister_candev(netdev);
kfree(dev->cmd_buf);
dev->next_siblings = NULL;
@@ -1102,6 +1085,7 @@ static int peak_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
const struct peak_usb_adapter *peak_usb_adapter;
+ struct peak_usb_device *dev;
int i, err = -ENOMEM;
/* get corresponding PCAN-USB adapter */
@@ -1116,13 +1100,40 @@ static int peak_usb_probe(struct usb_interface *intf,
for (i = 0; i < peak_usb_adapter->ctrl_count; i++) {
err = peak_usb_create_dev(peak_usb_adapter, intf, i);
+ if (err)
+ goto lbl_disconnect;
+ }
+
+ /* register the netdevs only once all the channels are set up: the
+ * channels of an interface share resources (command buffer, firmware
+ * info, ctrlmode copied from the 1st channel...) that must not be
+ * used through a netdev while a sibling is being initialized.
+ */
+ for (dev = usb_get_intfdata(intf); dev->prev_siblings;
+ dev = dev->prev_siblings)
+ ;
+
+ for (; dev; dev = dev->next_siblings) {
+ err = register_candev(dev->netdev);
if (err) {
- /* deregister already created devices */
- peak_usb_disconnect(intf);
- break;
+ dev_err(&intf->dev, "couldn't register CAN device: %d\n",
+ err);
+ goto lbl_disconnect;
}
+ set_bit(PCAN_USB_STATE_REGISTERED, &dev->state);
+
+ netdev_info(dev->netdev,
+ "attached to %s channel %u (device 0x%08X)\n",
+ peak_usb_adapter->name, dev->ctrl_idx,
+ dev->can_channel_id);
}
+ return 0;
+
+lbl_disconnect:
+ /* unregister (if needed) and free already created devices */
+ peak_usb_disconnect(intf);
+
return err;
}
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index cb8777c3e08e..614854c998de 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -114,6 +114,7 @@ struct peak_tx_urb_context {
*/
#define PCAN_USB_STATE_CONNECTED 0
#define PCAN_USB_STATE_STARTED 1
+#define PCAN_USB_STATE_REGISTERED 2
/* PEAK-System USB device */
struct peak_usb_device {
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v6 09/13] can: peak_usb: register the netdevs only once all channels are set up
2026-10-22 14:27 ` [PATCH v6 09/13] can: peak_usb: register the netdevs only once all channels are set up Stéphane Grosjean via B4 Relay
@ 2026-10-24 3:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 19+ messages in thread
From: netdev-bot+sashiko @ 2026-10-24 3:58 UTC (permalink / raw)
To: s.grosjean
Cc: mkl, mailhol, wg, error27, lukas.magel, linux-can, linux-kernel,
stable, kuba
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The patch only half-converts probe-time logging.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the PCAN-USB
private restart timer (struct pcan_usb::restart_timer, inside the…
- [Medium] This problem wasn't introduced by this patch, but the
mechanism is real.
--
can: peak_usb: register the netdevs only once all channels are set up
This moves register_candev() out of peak_usb_create_dev() and into
peak_usb_probe(). The netdevs of an interface are now registered only
after dev_init(), dev_set_bus() and dev_get_can_channel_id() have run for
every channel. peak_usb_disconnect() only unregisters netdevs flagged with
the new PCAN_USB_STATE_REGISTERED bit.
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 6a5ecb10c4f30..8a0a132983c44 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -390,9 +390,12 @@ static int pcan_usb_get_can_channel_id(struct peak_usb_device *dev, u32 *can_ch_
> int err;
>
> err = pcan_usb_wait_rsp(dev, PCAN_USB_CMD_DEVID, PCAN_USB_GET, args);
> + /* explicit use of dev_xxx() instead of netdev_xxx() here: this is
> + * also called during probe, before the netdev is registered.
> + */
> if (err)
> - netdev_err(dev->netdev, "getting can channel id failure: %d\n", err);
> -
> + dev_err(dev->netdev->dev.parent,
> + "getting can channel id failure: %d\n", err);
> else
> *can_ch_id = args[0];
[Severity: Low]
Should the helpers on this same path be converted as well?
pcan_usb_wait_rsp() and pcan_usb_send_cmd() still log with
netdev_err(dev->netdev, ...):
pcan_usb_wait_rsp() {
...
if (err)
netdev_err(dev->netdev,
"waiting rsp f=0x%x n=0x%x failure: %d\n", f, n, err);
...
}
dev_init(), dev_set_bus(dev, 0) and dev_get_can_channel_id() now all run
before register_candev(). At that point the netdev name is still the
"can%d" template and reg_state is NETREG_UNINITIALIZED. These messages
therefore print with an "(unnamed net_device) (uninitialized)" prefix. On
PCAN-USB, one failed channel id read now prints one such netdev_err()
followed by this dev_err().
The other adapters have the same problem in their command helpers, which
are reached from dev_init(), dev_set_bus() and dev_get_can_channel_id()
during probe:
- pcan_usb_pro_send_cmd(), pcan_usb_pro_wait_rsp() and
pcan_usb_pro_send_req() in pcan_usb_pro.c
- pcan_usb_fd_send_cmd() in pcan_usb_fd.c
The new lbl_disconnect path in peak_usb_probe() has a similar issue.
peak_usb_disconnect() copies netdev->name and logs "%s removed" for
channels that were created but never registered, so it prints a literal
"can%d removed".
> 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 49d2c01b8ef7e..ea6c28f3ed0b9 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[ ... ]
> @@ -1081,7 +1063,8 @@ static void peak_usb_disconnect(struct usb_interface *intf)
> dev_prev_siblings = dev->prev_siblings;
> strscpy(name, netdev->name, IFNAMSIZ);
>
> - unregister_candev(netdev);
> + if (test_bit(PCAN_USB_STATE_REGISTERED, &dev->state))
> + unregister_candev(netdev);
>
> kfree(dev->cmd_buf);
> dev->next_siblings = NULL;
[Severity: High]
This isn't a bug introduced by this patch, but can the PCAN-USB restart
timer fire after free_candev() in this loop?
struct pcan_usb::restart_timer sits in the netdev private area. It is set
up in pcan_usb_init(). It is armed by pcan_usb_restart_pending(), the
completion handler of the unanchored restart URB that
pcan_usb_restart_async() submits:
pcan_usb_restart_pending() {
/* the PCAN-USB needs time to restart */
mod_timer(&pdev->restart_timer,
jiffies + msecs_to_jiffies(PCAN_USB_STARTUP_TIMEOUT));
...
}
This handler ignores urb->status, so a URB flushed during unplug or unbind
still arms the timer.
Nothing cancels it:
- pcan_usb.c has no timer_delete_sync() or timer_shutdown_sync() on
restart_timer.
- The PCAN-USB adapter has no dev_stop, dev_free or dev_exit callback.
- close_candev() only cancels priv->restart_work.
Suppose a restart has happened (restart-ms auto-restart after bus-off, or a
manual "ip link set canX type can restart"). This sequence can then free
the memory that holds the queued timer_list:
peak_usb_disconnect()
unregister_candev()
peak_usb_ndo_stop()
free_candev()
When the timer expires, pcan_usb_restart()->peak_usb_restart_complete()
writes dev->can.state and calls netif_wake_queue() on the freed netdev.
[ ... ]
> @@ -1116,13 +1100,40 @@ static int peak_usb_probe(struct usb_interface *intf,
>
> for (i = 0; i < peak_usb_adapter->ctrl_count; i++) {
> err = peak_usb_create_dev(peak_usb_adapter, intf, i);
> + if (err)
> + goto lbl_disconnect;
> + }
> +
[ ... ]
> + for (; dev; dev = dev->next_siblings) {
> + err = register_candev(dev->netdev);
> if (err) {
> - /* deregister already created devices */
> - peak_usb_disconnect(intf);
> - break;
> + dev_err(&intf->dev, "couldn't register CAN device: %d\n",
> + err);
> + goto lbl_disconnect;
> }
[Severity: Medium]
This is a pre-existing issue, but take a multi-channel adapter (PCAN-USB
Pro, or a two-channel PCAN-USB FD). Channel 0 is registered and opened by
userspace, and then register_candev() fails for channel 1. What state is
the hardware left in?
The commit message says:
Since no netdev can be opened during probe anymore, there is no RX to
stop when probing a channel fails.
That doesn't quite hold inside this loop. Channel 0 can already be open,
through peak_usb_ndo_open()->dev_start()->dev_set_bus(dev, 1), before
channel 1's registration fails. RX is still stopped by peak_usb_stop_rx()
in peak_usb_disconnect(), so that part works.
However, peak_usb_disconnect() calls peak_usb_stop_rx() first, and that
clears PCAN_USB_STATE_CONNECTED on every sibling:
peak_usb_stop_rx() {
rtnl_lock();
for (d = dev; d; d = d->prev_siblings)
clear_bit(PCAN_USB_STATE_CONNECTED, &d->state);
rtnl_unlock();
...
}
Next, unregister_candev(ch0)->peak_usb_ndo_stop() calls dev_stop() and
dev_set_bus(dev, 0). Once CONNECTED is clear, pcan_usb_fd_send_cmd() and
pcan_usb_pro_send_cmd() return 0 without sending anything:
if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
dev_exit() (pcan_usb_fd_exit() or pcan_usb_pro_exit()) is the only place
that sends bus off, LED off, calibration off and drv_loaded(0). It is only
called from peak_usb_do_device_exit(), at module unload.
Does this leave the still-attached adapter bus-on on the opened channel?
Calibration messages would stay enabled, and the firmware would still
believe a driver is loaded.
The same ordering existed before this patch. A failure on channel 1 also
unwound through peak_usb_disconnect() after channel 0 might already have
been opened.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-canfd_check_channel_idx-v6-0-197704231551%40peak-system.fr
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 10/13] can: peak_usb: validate the data length of PCAN-USB Pro CAN records
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (8 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 09/13] can: peak_usb: register the netdevs only once all channels are set up Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 11/13] can: peak_usb: don't read past the message in PCAN-USB Pro decoders Stéphane Grosjean via B4 Relay
` (2 subsequent siblings)
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel, stable
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
The PCAN-USB Pro decoder takes the data length of a received CAN frame
from the 4-bit DLC field of the record, i.e. 0 to 15, and copies that
many bytes from the record into can_frame->data. This length is checked
neither against CAN_MAX_DLEN nor against the size of the record, which
depends on its type: RXMSG8, RXMSG4 and RXMSG0/RXRTR records carry 8, 4
and 0 data bytes. pcan_usb_pro_decode_buf() only checks that the record
fits in the URB, according to its type.
A faulty or malicious device can therefore make the driver:
- copy bytes located beyond the record into the frame, i.e. stale bytes
of the RX buffer or, when the record ends the 1024-byte buffer, bytes
beyond it, and deliver them to the CAN sockets;
- write up to 7 bytes past can_frame->data[] with a DLC of 9 to 15;
- deliver an RTR frame whose len is greater than CAN_MAX_DLEN.
Convert the DLC with can_cc_dlc2len() and reject a data frame whose
length exceeds the number of data bytes of its record type, as the
other malformed records are.
Found by Sashiko AI review.
Fixes: d8a199355f8f ("can: usb: PEAK-System Technik PCAN-USB Pro specific part")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 151c5ea2859b..f94494e01f4a 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -534,6 +534,7 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
struct pcan_usb_pro_rxmsg *rx)
{
const unsigned int ctrl_idx = (rx->len >> 4) & 0x0f;
+ const u8 len = can_cc_dlc2len(rx->len & 0x0f);
struct peak_usb_device *dev;
struct net_device *netdev;
struct can_frame *can_frame;
@@ -543,6 +544,12 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
return -EINVAL;
+ /* the number of data bytes depends on the record type */
+ if (!(rx->flags & PCAN_USBPRO_RTR) &&
+ len > pcan_usb_pro_sizeof_rec[rx->data_type] -
+ offsetof(struct pcan_usb_pro_rxmsg, data))
+ return -EINVAL;
+
dev = READ_ONCE(usb_if->dev[ctrl_idx]);
/* reject records for a channel that is not (or no longer) set up */
@@ -556,7 +563,7 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
return -ENOMEM;
can_frame->can_id = le32_to_cpu(rx->id);
- can_frame->len = rx->len & 0x0f;
+ can_frame->len = len;
if (rx->flags & PCAN_USBPRO_EXT)
can_frame->can_id |= CAN_EFF_FLAG;
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 11/13] can: peak_usb: don't read past the message in PCAN-USB Pro decoders
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (9 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 10/13] can: peak_usb: validate the data length of PCAN-USB Pro CAN records Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-24 3:58 ` netdev-bot+sashiko
2026-10-22 14:27 ` [PATCH v6 12/13] can: peak_usb: dump undecodable URBs once and rate limit decode errors Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 13/13] can: peak_usb: don't return an uninitialized PCAN-USB CAN channel ID Stéphane Grosjean via B4 Relay
12 siblings, 1 reply; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
The messages received from a PCAN-USB Pro device start with a record
count, followed by records whose size depends on their type, given by
their first byte. Both pcan_usb_pro_decode_buf() and
pcan_usb_pro_wait_rsp() trust this count:
- pcan_usb_pro_decode_buf() reads the type of the next record before
checking that this record starts inside the URB. When the device
claims more records than the URB holds, it reads the byte following
the URB data, which is beyond the 1024-byte RX buffer when the URB
fills it;
- pcan_usb_pro_wait_rsp() doesn't check the records against the
received length at all. Since the count is a 32-bit value, a bogus
response makes it walk the records past the command buffer.
Stop decoding a message at its end in both functions, and in
pcan_usb_pro_wait_rsp() also reject a record that doesn't fit in the
message.
Found by Sashiko AI review.
Fixes: d8a199355f8f ("can: usb: PEAK-System Technik PCAN-USB Pro specific part")
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index f94494e01f4a..b869a20438cf 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -244,9 +244,9 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
for (i = 0; !err && i < PCAN_USBPRO_RSP_SUBMIT_MAX; i++) {
struct pcan_usb_pro_msg rsp;
union pcan_usb_pro_rec *pr;
+ u8 *pc, *rsp_end;
u32 r, rec_cnt;
u16 rec_len;
- u8 *pc;
err = usb_bulk_msg(dev->udev,
usb_rcvbulkpipe(dev->udev, PCAN_USBPRO_EP_CMDIN),
@@ -271,13 +271,18 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
pc = pcan_msg_init(&rsp, pum->u.rec_buffer,
actual_length);
+ rsp_end = pum->u.rec_buffer + actual_length;
rec_cnt = le32_to_cpu(*rsp.u.rec_cnt);
/* loop on records stored into message */
for (r = 0; r < rec_cnt; r++) {
+ /* rec_cnt comes from the device: stay in the message */
+ if (pc >= rsp_end)
+ break;
+
pr = (union pcan_usb_pro_rec *)pc;
rec_len = pcan_usb_pro_sizeof_rec[pr->data_type];
- if (!rec_len) {
+ if (!rec_len || pc + rec_len > rsp_end) {
netdev_err(dev->netdev,
"got unprocessed record in msg\n");
pcan_dump_mem("rcvd rsp msg", pum->u.rec_buffer,
@@ -745,8 +750,15 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
rec_cnt = le16_to_cpu(*usb_msg.u.rec_cnt_rd);
for (; rec_cnt > 0; rec_cnt--) {
union pcan_usb_pro_rec *pr = (union pcan_usb_pro_rec *)rec_ptr;
- u16 sizeof_rec = pcan_usb_pro_sizeof_rec[pr->data_type];
+ u16 sizeof_rec;
+
+ /* rec_cnt comes from the device: don't read past the message */
+ if (rec_ptr >= msg_end) {
+ err = -EBADMSG;
+ break;
+ }
+ sizeof_rec = pcan_usb_pro_sizeof_rec[pr->data_type];
if (!sizeof_rec) {
netdev_err(netdev,
"got unsupported rec in usb msg:\n");
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v6 11/13] can: peak_usb: don't read past the message in PCAN-USB Pro decoders
2026-10-22 14:27 ` [PATCH v6 11/13] can: peak_usb: don't read past the message in PCAN-USB Pro decoders Stéphane Grosjean via B4 Relay
@ 2026-10-24 3:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 19+ messages in thread
From: netdev-bot+sashiko @ 2026-10-24 3:58 UTC (permalink / raw)
To: s.grosjean
Cc: mkl, mailhol, wg, error27, lukas.magel, linux-can, linux-kernel,
stable, kuba
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message overstates what the old
pcan_usb_pro_wait_rsp() bug could do.
--
can: peak_usb: don't read past the message in PCAN-USB Pro decoders
This commit adds bounds checks to pcan_usb_pro_decode_buf() and
pcan_usb_pro_wait_rsp(). With these checks, the record walk stops at the
end of the received message and no longer trusts the record count sent by
the device. pcan_usb_pro_wait_rsp() also now rejects a record that doesn't
fit in the received length.
> - pcan_usb_pro_wait_rsp() doesn't check the records against the
> received length at all. Since the count is a 32-bit value, a bogus
> response makes it walk the records past the command buffer.
[Severity: Low]
With the current callers, is the last sentence accurate? The code change
looks correct. It is less clear that the old walk could ever leave the
command buffer allocation.
pcan_usb_pro_get_can_channel_id() in
drivers/net/can/usb/peak_usb/pcan_usb_pro.c is the only caller of
pcan_usb_pro_wait_rsp(). It builds a 4-byte header plus an 8-byte
GETDEVID record in dev->cmd_buf:
pcan_usb_pro_get_can_channel_id() {
...
pc = pcan_msg_init_empty(&um, dev->cmd_buf, PCAN_USB_MAX_CMD_LEN);
pcan_msg_add_rec(&um, PCAN_USBPRO_GETDEVID, dev->ctrl_idx);
...
}
That makes um.rec_buffer_len 12. pcan_usb_pro_wait_rsp() passes that
length to usb_bulk_msg(), so the device can write at most 12 bytes.
peak_usb_create_dev() allocates dev->cmd_buf with
kzalloc(PCAN_USB_MAX_CMD_LEN), and PCAN_USB_MAX_CMD_LEN is 32. None of the
PCAN-USB Pro command builders write more than 12 bytes into it, so bytes
12-31 stay zero.
No record type 0x00 is defined, so pcan_usb_pro_sizeof_rec[0] is 0. All
record sizes are multiples of 4, and the largest is RXMSG8 at 20 bytes.
The old loop therefore reads type bytes the device controls only at
offsets 4 and 8. From offset 8 it can advance to offset 28 at most. There
it reads a zero type and stops on the !rec_len check, whatever value
rec_cnt has.
What the old code could do is read past actual_length. For example, it
could accept a truncated response with actual_length == 8 and a GETDEVID
record at offset 4. In that case it would return the stale request bytes
8-11 as dev_num. The new "pc + rec_len > rsp_end" check fixes that case.
Could the commit message describe this case instead, so that the stated
impact matches what the old pcan_usb_pro_wait_rsp() code could actually
do?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-canfd_check_channel_idx-v6-0-197704231551%40peak-system.fr
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 12/13] can: peak_usb: dump undecodable URBs once and rate limit decode errors
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (10 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 11/13] can: peak_usb: don't read past the message in PCAN-USB Pro decoders Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
2026-10-22 14:27 ` [PATCH v6 13/13] can: peak_usb: don't return an uninitialized PCAN-USB CAN channel ID Stéphane Grosjean via B4 Relay
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
When the decoding of a received URB fails, peak_usb_read_bulk_callback()
dumps the whole RX buffer to the kernel log. The buffer is allocated with
kmalloc() and its size is the adapter RX buffer size (up to 2048 bytes),
so the bytes beyond urb->actual_length are uninitialized memory or
leftovers from previous transfers.
Moreover, the PCAN-USB Pro and PCAN-USB FD decoders already dump the
received bytes themselves before returning the error, so the same URB is
dumped twice. Neither these dumps nor the error messages printed by the
decoders are rate limited, while a faulty or malicious device can make
every transfer fail to decode, or carry unknown records.
Dump only the urb->actual_length bytes received, from
peak_usb_read_bulk_callback() only, and rate limit this dump as well as
the error messages the decoders print about the received data.
The dumps and messages of the adapter decoders were pointed out by
Sashiko AI review.
Fixes: bb4785551f64 ("can: usb: PEAK-System Technik USB adapters driver core")
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 8 +++++---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 4 ++--
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 27 ++++++++++++-------------
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 30 ++++++++++++++--------------
4 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 8a0a132983c4..04019a9b868e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -637,7 +637,8 @@ static int pcan_usb_decode_status(struct pcan_usb_msg_context *mc,
return err;
break;
default:
- netdev_err(mc->netdev, "unexpected function %u\n", f);
+ if (net_ratelimit())
+ netdev_err(mc->netdev, "unexpected function %u\n", f);
break;
}
@@ -776,8 +777,9 @@ static int pcan_usb_decode_buf(struct peak_usb_device *dev, struct urb *urb)
urb->actual_length);
} else if (urb->actual_length > 0) {
- netdev_err(dev->netdev, "usb message length error (%u)\n",
- urb->actual_length);
+ if (net_ratelimit())
+ netdev_err(dev->netdev, "usb message length error (%u)\n",
+ urb->actual_length);
err = -EINVAL;
}
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 ea6c28f3ed0b..67457d094b60 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -266,10 +266,10 @@ static void peak_usb_read_bulk_callback(struct urb *urb)
/* handle these kinds of msgs only if _start callback called */
if (test_bit(PCAN_USB_STATE_STARTED, &dev->state)) {
err = dev->adapter->dev_decode_buf(dev, urb);
- if (err)
+ if (err && net_ratelimit())
pcan_dump_mem("received usb message",
urb->transfer_buffer,
- urb->transfer_buffer_length);
+ urb->actual_length);
}
}
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index eee5b5eaa247..9c34f431f773 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -796,15 +796,17 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
/* check if the record goes out of current packet */
if (rx_msg_size > msg_end - msg_ptr) {
- netdev_err(netdev,
- "got frag rec: should inc usb rx buf sze\n");
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "got frag rec: should inc usb rx buf sze\n");
err = -EBADMSG;
break;
}
rx_msg_min_size = pcan_usb_fd_rx_msg_min_size(rx_msg_type);
if (rx_msg_size < rx_msg_min_size) {
- netdev_err(netdev, "got short rec\n");
+ if (net_ratelimit())
+ netdev_err(netdev, "got short rec\n");
err = -EBADMSG;
break;
}
@@ -813,7 +815,7 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
case PUCAN_MSG_CAN_RX:
err = pcan_usb_fd_decode_canmsg(usb_if, rx_msg);
if (err < 0)
- goto fail;
+ return err;
break;
case PCAN_UFD_MSG_CALIBRATION:
@@ -823,35 +825,32 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
case PUCAN_MSG_ERROR:
err = pcan_usb_fd_decode_error(usb_if, rx_msg);
if (err < 0)
- goto fail;
+ return err;
break;
case PUCAN_MSG_STATUS:
err = pcan_usb_fd_decode_status(usb_if, rx_msg);
if (err < 0)
- goto fail;
+ return err;
break;
case PCAN_UFD_MSG_OVERRUN:
err = pcan_usb_fd_decode_overrun(usb_if, rx_msg);
if (err < 0)
- goto fail;
+ return err;
break;
default:
- netdev_err(netdev,
- "unhandled msg type 0x%02x (%d): ignored\n",
- rx_msg_type, rx_msg_type);
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "unhandled msg type 0x%02x (%d): ignored\n",
+ rx_msg_type, rx_msg_type);
break;
}
msg_ptr += rx_msg_size;
}
-fail:
- if (err)
- pcan_dump_mem("received msg",
- urb->transfer_buffer, urb->actual_length);
return err;
}
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index b869a20438cf..9fd603a868d6 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -741,7 +741,9 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
rec_ptr = pcan_msg_init(&usb_msg, urb->transfer_buffer,
urb->actual_length);
if (!rec_ptr) {
- netdev_err(netdev, "bad msg hdr len %d\n", urb->actual_length);
+ if (net_ratelimit())
+ netdev_err(netdev, "bad msg hdr len %d\n",
+ urb->actual_length);
return -EINVAL;
}
@@ -760,16 +762,18 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
sizeof_rec = pcan_usb_pro_sizeof_rec[pr->data_type];
if (!sizeof_rec) {
- netdev_err(netdev,
- "got unsupported rec in usb msg:\n");
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "got unsupported rec in usb msg:\n");
err = -ENOTSUPP;
break;
}
/* check if the record goes out of current packet */
if (rec_ptr + sizeof_rec > msg_end) {
- netdev_err(netdev,
- "got frag rec: should inc usb rx buf size\n");
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "got frag rec: should inc usb rx buf size\n");
err = -EBADMSG;
break;
}
@@ -781,13 +785,13 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
case PCAN_USBPRO_RXRTR:
err = pcan_usb_pro_handle_canmsg(usb_if, &pr->rx_msg);
if (err < 0)
- goto fail;
+ return err;
break;
case PCAN_USBPRO_RXSTATUS:
err = pcan_usb_pro_handle_error(usb_if, &pr->rx_status);
if (err < 0)
- goto fail;
+ return err;
break;
case PCAN_USBPRO_RXTS:
@@ -795,20 +799,16 @@ static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, struct urb *urb)
break;
default:
- netdev_err(netdev,
- "unhandled rec type 0x%02x (%d): ignored\n",
- pr->data_type, pr->data_type);
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "unhandled rec type 0x%02x (%d): ignored\n",
+ pr->data_type, pr->data_type);
break;
}
rec_ptr += sizeof_rec;
}
-fail:
- if (err)
- pcan_dump_mem("received msg",
- urb->transfer_buffer, urb->actual_length);
-
return err;
}
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v6 13/13] can: peak_usb: don't return an uninitialized PCAN-USB CAN channel ID
2026-10-22 14:27 [PATCH v6 00/13] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean via B4 Relay
` (11 preceding siblings ...)
2026-10-22 14:27 ` [PATCH v6 12/13] can: peak_usb: dump undecodable URBs once and rate limit decode errors Stéphane Grosjean via B4 Relay
@ 2026-10-22 14:27 ` Stéphane Grosjean via B4 Relay
12 siblings, 0 replies; 19+ messages in thread
From: Stéphane Grosjean via B4 Relay @ 2026-10-22 14:27 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Wolfgang Grandegger,
Stéphane Grosjean, Dan Carpenter, Lukas Magel
Cc: linux-can, linux-kernel, stable
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
pcan_usb_get_can_channel_id() takes the CAN channel ID of a PCAN-USB
from the args[] array, which pcan_usb_wait_rsp() fills with the response
of the device. However, args[] is not initialized, and
pcan_usb_wait_rsp() returns success without touching it once the device
is no longer connected.
peak_usb_disconnect() clears PCAN_USB_STATE_CONNECTED before it
unregisters the netdevs. An ETHTOOL_GEEPROM request handled in this
window makes peak_usb_get_eeprom() copy an uninitialized stack byte to
userspace, and cache it in dev->can_channel_id.
Initialize args[], so that a null ID is returned in that case.
Found by Sashiko AI review.
Fixes: 36d007c6fc79 ("can: peak_usb: add ethtool interface to user-configurable CAN channel identifier")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 04019a9b868e..4d8f23f2fa2f 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -386,7 +386,7 @@ static int pcan_usb_get_serial(struct peak_usb_device *dev, u32 *serial_number)
*/
static int pcan_usb_get_can_channel_id(struct peak_usb_device *dev, u32 *can_ch_id)
{
- u8 args[PCAN_USB_CMD_ARGS_LEN];
+ u8 args[PCAN_USB_CMD_ARGS_LEN] = {};
int err;
err = pcan_usb_wait_rsp(dev, PCAN_USB_CMD_DEVID, PCAN_USB_GET, args);
--
2.53.0
^ permalink raw reply [flat|nested] 19+ messages in thread