* [PATCH v3 01/10] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 02/10] can: peak_usb: add missing includes Stéphane Grosjean
` (8 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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] 16+ messages in thread* [PATCH v3 02/10] can: peak_usb: add missing includes
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 01/10] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 03/10] can: peak_usb: sort ctrlmode_supported flags Stéphane Grosjean
` (7 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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] 16+ messages in thread* [PATCH v3 03/10] can: peak_usb: sort ctrlmode_supported flags
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 01/10] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 02/10] can: peak_usb: add missing includes Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 04/10] can: peak_usb: harden PCAN-USB message validation Stéphane Grosjean
` (6 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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] 16+ messages in thread* [PATCH v3 04/10] can: peak_usb: harden PCAN-USB message validation
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (2 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 03/10] can: peak_usb: sort ctrlmode_supported flags Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 05/10] can: peak_usb: replace open-coded bitmask operations with bitops Stéphane Grosjean
` (5 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, linux-can, linux-kernel
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")
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] 16+ messages in thread* [PATCH v3 05/10] can: peak_usb: replace open-coded bitmask operations with bitops
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (3 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 04/10] can: peak_usb: harden PCAN-USB message validation Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 06/10] can: peak_usb: harden channel handling in PCAN-USB FD Stéphane Grosjean
` (4 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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] 16+ messages in thread* [PATCH v3 06/10] can: peak_usb: harden channel handling in PCAN-USB FD
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (4 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 05/10] can: peak_usb: replace open-coded bitmask operations with bitops Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-21 22:58 ` netdev-bot+sashiko
2026-10-20 9:08 ` [PATCH v3 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR Stéphane Grosjean
` (3 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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.
Fixes: a6921dd524fe ("can: peak_usb: add range checking in decode operations")
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 30 +++++++++++++-
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 62 +++++++++++++++++++++-------
2 files changed, 75 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..4c08656e7768 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -574,6 +574,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 +907,22 @@ 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.
+ */
+static void peak_usb_stop_rx(struct peak_usb_device *dev)
+{
+ struct peak_usb_device *d;
+
+ for (d = dev; d; d = d->prev_siblings)
+ clear_bit(PCAN_USB_STATE_CONNECTED, &d->state);
+
+ 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 +1033,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 +1063,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] 16+ messages in thread* Re: [PATCH v3 06/10] can: peak_usb: harden channel handling in PCAN-USB FD
2026-10-20 9:08 ` [PATCH v3 06/10] can: peak_usb: harden channel handling in PCAN-USB FD Stéphane Grosjean
@ 2026-10-21 22:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 16+ messages in thread
From: netdev-bot+sashiko @ 2026-10-21 22:58 UTC (permalink / raw)
To: stephane.grosjean
Cc: mkl, mailhol, error27, wg, s.grosjean, linux-can, linux-kernel, kuba
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (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).
- [High] At this commit, the adap_dev_free error path in
peak_usb_create_dev() (drivers/net/can/usb/peak_usb/pcan_usb_core.c)…
Pre-existing issues:
- [Medium] When peak_usb_disconnect() runs while the adapter is still
powered (probe failure or sysfs unbind), PCAN_USB_STATE_CONNECTED is…
--
can: peak_usb: harden channel handling in PCAN-USB FD
This commit checks the channel number in records from PCAN-USB FD devices
against the adapter's controller count. Records for an empty channel slot
are now rejected. It also clears the channel slot in pcan_usb_fd_free(),
stops RX on the sibling channels when a probe fails, and makes
peak_usb_disconnect() stop RX on all channels before it unregisters any
of them.
> 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..4c08656e77681 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[ ... ]
> @@ -904,6 +907,22 @@ 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.
> + */
> +static void peak_usb_stop_rx(struct peak_usb_device *dev)
> +{
> + struct peak_usb_device *d;
> +
> + for (d = dev; d; d = d->prev_siblings)
> + clear_bit(PCAN_USB_STATE_CONNECTED, &d->state);
> +
> + for (d = dev; d; d = d->prev_siblings)
> + usb_poison_anchored_urbs(&d->rx_submitted);
> +}
> +
[ ... ]
> @@ -1014,6 +1033,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);
>
[Severity: High]
Does this error path leave the failing channel running on memory that
dev_free() has just released?
peak_usb_stop_rx() only walks dev->prev_siblings. So dev keeps
PCAN_USB_STATE_CONNECTED set, and its rx_submitted URBs are not poisoned.
On a single-channel adapter, or when channel 0 fails, prev_siblings is
NULL and nothing is stopped.
register_candev() runs before dev_init() and dev_set_bus(), so the
netdev may already be open when dev_set_bus(dev, 0) fails. In that case
pcan_usb_fd_free() finds no siblings and frees the shared objects:
pcan_usb_fd_free() {
...
if (!dev->prev_siblings && !dev->next_siblings) {
kfree(pdev->cmd_buffer_addr);
kfree(pdev->usb_if);
}
}
Only after that does the code fall through to:
lbl_unregister_candev:
unregister_candev(netdev);
In the meantime, dev's RX completions can keep running
pcan_usb_fd_decode_buf() on the freed usb_if. The decoders read
usb_if->adapter and usb_if->dev[], and the timestamp decoding writes
cm_ignore_count and time_ref.
After that, unregister_candev()->peak_usb_ndo_stop()->pcan_usb_fd_stop()
does:
if (pdev->usb_if->dev_opened_count == 1)
pcan_usb_fd_set_options(dev, 0,
PUCAN_OPTION_ERROR,
PCAN_UFD_FLTEXT_CALIBRATION);
pdev->usb_if->dev_opened_count--;
This reads and writes the freed usb_if. pcan_usb_fd_set_options() and
pcan_usb_fd_set_bus() also build commands in the freed cmd_buffer_addr.
The commit message says RX is stopped before the channel is freed, but
here that only covers the siblings.
A later patch in this series, "can: peak_usb: unregister the netdev
before dev_free() on probe failure", moves unregister_candev(netdev) in
front of dev_free(dev). With that change, ndo_stop kills
dev->rx_submitted while usb_if is still allocated. Could that ordering be
folded into this patch, so this commit is not left with the
use-after-free?
> @@ -1038,13 +1063,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, and this patch did not introduce it.
peak_usb_disconnect() can run while the adapter is still powered, from a
probe failure or a sysfs unbind. CONNECTED is cleared before
unregister_candev(). Are the bus-off commands sent from ndo_stop dropped
in that case?
pcan_usb_fd_send_cmd() returns success without sending anything:
/* usb device unregistered? */
if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return 0;
So two commands never reach the device: PUCAN_CMD_RESET_MODE from
peak_usb_ndo_stop()->pcan_usb_fd_set_bus(dev, 0), and the options-off
command from pcan_usb_fd_stop(). The CAN controller could then stay in
operational mode with no driver bound.
The old code already cleared CONNECTED just before each channel's
unregister_candev(), so the behavior is the same. This patch only clears
the bit on all channels up front.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v3-0-74d17c5a599e%40peak-system.fr
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (5 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 06/10] can: peak_usb: harden channel handling in PCAN-USB FD Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-21 22:58 ` netdev-bot+sashiko
2026-10-20 9:08 ` [PATCH v3 08/10] can: peak_usb: reject records for unset channels in PCAN-USB Pro Stéphane Grosjean
` (2 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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.
Fixes: 3a7939495ce8 ("can: peak_usb: PCAN-USB: add support of loopback and one-shot mode")
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 | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 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..4c8cfed31634 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -697,16 +697,19 @@ 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;
- /* 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++;
+ }
+
mc->netdev->stats.rx_packets++;
/* convert timestamp into kernel time */
--
2.53.0
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR
2026-10-20 9:08 ` [PATCH v3 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR Stéphane Grosjean
@ 2026-10-21 22:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 16+ messages in thread
From: netdev-bot+sashiko @ 2026-10-21 22:58 UTC (permalink / raw)
To: stephane.grosjean
Cc: mkl, mailhol, error27, wg, s.grosjean, linux-can, linux-kernel, 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 rx_bytes counter in pcan_usb_decode_data()
(drivers/net/can/usb/peak_usb/pcan_usb.c) can now count bytes from a…
- [Low] This patch is tagged `Cc: stable@vger.kernel.org` with `Fixes:
3a7939495ce8`, but it is written against the tree after the earlier…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
pcan_usb_encode_msg() and pcan_usb_decode_data() use different payload…
--
can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR
This commit moves the skip of the SRR client private id byte in
pcan_usb_decode_data() out of the non-RTR branch. The byte is now skipped
whenever PCAN_USB_TX_SRR is set. This matches pcan_usb_encode_msg(), which
appends the writer id byte for both RTR and non-RTR frames.
> Fixes: 3a7939495ce8 ("can: peak_usb: PCAN-USB: add support of loopback and one-shot mode")
> Cc: stable@vger.kernel.org
[Severity: Low]
Will this apply cleanly to stable trees? The hunk removed here is the
braced form that includes the mc->end bounds check:
if (can_id_flags & PCAN_USB_TX_SRR) {
if ((mc->ptr + 1) > mc->end)
goto decode_failed;
mc->ptr++;
}
That bounds check comes from the earlier patch in this series, "can:
peak_usb: harden PCAN-USB message validation", and that patch is not
tagged for stable. Stable trees that have 3a7939495ce8 but not the
hardening patch still have:
if (can_id_flags & PCAN_USB_TX_SRR)
mc->ptr++;
The hardening patch has a Fixes: tag, so it may get picked up anyway.
Even so, should this patch name the prerequisite, for example with a
"Cc: stable@vger.kernel.org # <prereq>" annotation as described in
stable-kernel-rules? Or should the hardening patch also be Cc: stable?
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 93da0fd4f10e7..4c8cfed31634a 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -697,16 +697,19 @@ 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;
[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Can
the RX cursor overshoot here when the DLC nibble is 9..15?
pcan_usb advertises CAN_CTRLMODE_CC_LEN8_DLC. In that mode rec_len is
status_len & PCAN_USB_STATUSLEN_DLC, which can be up to 15, and
can_frame_set_cc_len() caps cf->len at 8. mc->ptr is then advanced by
rec_len, not by cf->len.
On the TX side, pcan_usb_encode_msg() writes the raw DLC into the
status/len byte. It then copies only cf->len data bytes before the SRR
writer id:
/* status/len byte */
*pc = can_get_cc_dlc(cf, dev->can.ctrlmode);
...
/* can data */
if (!(cf->can_id & CAN_RTR_FLAG)) {
memcpy(pc, cf->data, cf->len);
pc += cf->len;
}
Suppose the device puts only 8 payload bytes in an RX record with a DLC
of 9..15. The cursor would then skip rec_len - 8 bytes of the next
record. That is the same kind of desync this patch fixes for RTR+SRR.
This depends on the firmware's RX record layout, which I could not
confirm from the code. The mc->ptr += rec_len line dates back to
46be265d3388. The len8_dlc handling came with 4c01fc87675e ("can:
drivers: add len8_dlc support for various CAN adapters"), which carries a
Tested-by.
>
> - /* 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++;
> + }
[Severity: Low]
Can rx_bytes now count bytes from a frame that is then dropped?
Before this patch, the SRR bounds check in the non-RTR branch ran before
the rx_bytes update. Take a non-RTR record with PCAN_USB_TX_SRR set whose
URB data ends right after the payload. pcan_usb_decode_data() now does:
memcpy(cf->data, mc->ptr, cf->len);
mc->ptr += rec_len;
mc->netdev->stats.rx_bytes += cf->len;
...
if ((mc->ptr + 1) > mc->end)
goto decode_failed;
decode_failed frees the skb, and rx_packets++ is skipped. rx_bytes is
then out of step with rx_packets by up to 8 bytes. Hitting this needs a
truncated URB from a faulty or malicious device.
Would it make sense to move the rx_bytes update after the SRR check, next
to rx_packets++?
> +
> mc->netdev->stats.rx_packets++;
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v3-0-74d17c5a599e%40peak-system.fr
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 08/10] can: peak_usb: reject records for unset channels in PCAN-USB Pro
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (6 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-21 22:58 ` netdev-bot+sashiko
2026-10-20 9:08 ` [PATCH v3 09/10] can: peak_usb: unregister the netdev before dev_free() on probe failure Stéphane Grosjean
2026-10-20 9:08 ` [PATCH v3 10/10] can: peak_usb: only dump received bytes on decode error Stéphane Grosjean
9 siblings, 1 reply; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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")
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] 16+ messages in thread* Re: [PATCH v3 08/10] can: peak_usb: reject records for unset channels in PCAN-USB Pro
2026-10-20 9:08 ` [PATCH v3 08/10] can: peak_usb: reject records for unset channels in PCAN-USB Pro Stéphane Grosjean
@ 2026-10-21 22:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 16+ messages in thread
From: netdev-bot+sashiko @ 2026-10-21 22:58 UTC (permalink / raw)
To: stephane.grosjean
Cc: mkl, mailhol, error27, wg, s.grosjean, linux-can, linux-kernel, 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 fix for the stale slot after a failed probe of channel 1
relies on core code from an earlier patch, and the commit message does…
- [Low] In pcan_usb_pro_handle_canmsg() and pcan_usb_pro_handle_error(),
a record for a channel whose slot is still NULL now returns -EINVAL.
- [Low] The header comment on pcan_usb_pro_free() still reads 'called
when PCAN-USB Pro adapter is unplugged'.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
peak_usb_create_dev() calls register_candev(netdev) (line 1001) before…
- [High] At this commit, if dev_set_bus(dev, 0) fails while probing
channel 0, the adap_dev_free path in peak_usb_create_dev() calls…
- [High] This problem wasn't introduced by this patch, but
pcan_usb_pro_handle_canmsg() trusts the DLC nibble from the device.
--
can: peak_usb: reject records for unset channels in PCAN-USB Pro
This patch makes the PCAN-USB Pro RX decoders reject CAN and status
records whose channel slot in usb_if->dev[] is still NULL, instead of
dereferencing it. The slot is published with smp_store_release() in
pcan_usb_pro_init() and cleared in pcan_usb_pro_free(), so a failed probe
of channel 1 no longer leaves a stale pointer behind.
> 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: Low]
Should an empty slot count as a fatal decode error?
pcan_usb_pro_decode_buf() treats any negative return as fatal:
case PCAN_USBPRO_RXRTR:
err = pcan_usb_pro_handle_canmsg(usb_if, &pr->rx_msg);
if (err < 0)
goto fail;
The goto skips rec_ptr += sizeof_rec and leaves the rec_cnt loop. That
drops every remaining record in the same bulk-in transfer, including
channel-0 CAN frames, channel-0 RXSTATUS records (a BUS_OFF transition,
for example) and RXTS calibration records.
The fail path then dumps the whole buffer with pcan_dump_mem(), which is
not rate limited. peak_usb_read_bulk_callback() dumps it again. A later
patch in the series rate limits only that second dump.
Before this patch, -EINVAL here meant a malformed record (ctrl_idx out of
range). An empty slot is an expected, temporary state for a well-formed
record, in exactly the window the commit message describes. The default
case in pcan_usb_pro_decode_buf() skips records it does not handle.
This is still better than the NULL dereference it replaces. Would
skipping the record (returning 0) fit better, though? The same question
applies to the new check in pcan_usb_pro_handle_error().
If stopping is intended, could the commit message say so? It only says
"Reject records", while the sibling FD commit says the decoder stops
processing the rest of the URB.
> +
> netdev = dev->netdev;
>
> skb = alloc_can_skb(netdev, &can_frame);
[Severity: High]
This is a pre-existing issue, but since this patch hardens
pcan_usb_pro_handle_canmsg() against untrusted channel numbers, what
about the DLC nibble from the same record?
can_frame->len = rx->len & 0x0f;
...
memcpy(can_frame->data, rx->data, can_frame->len);
can_frame->len can be anything from 0 to 15 and is not checked against
CAN_MAX_DLEN. can_frame->data and rx->data are both 8 bytes.
The DLC is also not checked against the real record size.
pcan_usb_pro_decode_buf() only checks rec_ptr + sizeof_rec > msg_end,
using pcan_usb_pro_sizeof_rec[]. That is 20 bytes for RXMSG8, 16 for
RXMSG4, and 12 for RXMSG0/RXRTR, which carry no data.
So a broken or malicious device could:
(a) put an RXMSG0 record with the RTR flag clear at the end of a full
1024-byte transfer. It passes the bound check, and memcpy() reads up to
15 bytes past the kmalloc(PCAN_USBPRO_RX_BUFFER_SIZE) buffer allocated
in peak_usb_start().
(b) send a short record with DLC <= 8. This copies bytes from beyond the
record (stale kmalloc contents past actual_length, or a neighboring slab
object) into a valid CAN frame, and CAN_RAW delivers that frame to
unprivileged sockets.
(c) use DLC 9..15. This writes up to 7 bytes past can_frame->data, which
FORTIFY_SOURCE reports as a field-spanning write.
Should the DLC be clamped to CAN_MAX_DLEN and checked against the record
type's payload size? No later patch in the series touches
pcan_usb_pro.c.
[ ... ]
> @@ -933,7 +943,8 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
> }
>
> pdev->usb_if = usb_if;
[Severity: High]
This isn't a bug introduced by this patch, but pdev->usb_if is only set
here, and pcan_usb_pro_init() runs after the netdev is registered. Can
pcan_usb_pro_start() dereference a NULL usb_if?
peak_usb_create_dev() does:
err = register_candev(netdev);
...
if (dev->adapter->dev_init) {
err = dev->adapter->dev_init(dev);
Once the netdev is registered, a privileged process, or a network
manager reacting to RTM_NEWLINK, can set the bitrate and bring the
interface up. peak_usb_ndo_open() only checks PCAN_USB_STATE_CONNECTED,
and that bit is set before registration:
peak_usb_ndo_open()
peak_usb_start()
pcan_usb_pro_start()
if (pdev->usb_if->dev_opened_count == 0) {
For channel 0, the window covers the synchronous USB requests in
pcan_usb_pro_init() (firmware info, bootloader info, drv_loaded). The
oops happens under rtnl_lock, so RTNL is never released.
The new slot check does not cover this, because the NULL pointer is
usb_if itself. The series does not fix it either.
> - 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)
> {
[Severity: Low]
This isn't a bug, but the comment above pcan_usb_pro_free() still says:
/*
* called when PCAN-USB Pro adapter is unplugged
*/
The slot clearing added here exists for the other caller, the probe
failure path in peak_usb_create_dev(). pcan_usb_fd_free() uses the same
slot scheme, and its comment was updated to "called when the USB adapter
is unplugged, or when probing a sibling fails". Should this comment be
updated the same way?
> + /* clear this channel's slot: dev is going to be freed */
> + WRITE_ONCE(pcan_usb_pro_dev_if(dev)->dev[dev->ctrl_idx], NULL);
[Severity: Low]
The commit message says this handles the case where "probing channel 1
fails after its slot has been filled". Is clearing the slot enough by
itself?
A channel-0 RX completion that loaded dev[1] before this WRITE_ONCE()
can still use the channel-1 peak_usb_device after free_candev().
What closes that window is the peak_usb_stop_rx() call in the
adap_dev_free path of peak_usb_create_dev(). It poisons channel 0's RX
anchor and waits for running completions:
adap_dev_free:
...
peak_usb_stop_rx(dev->prev_siblings);
if (dev->adapter->dev_free)
dev->adapter->dev_free(dev);
That helper and this call came from the earlier commit "can: peak_usb:
harden channel handling in PCAN-USB FD". Its subject only names the FD
adapter, and its Fixes: tag (a6921dd524fe) differs from the one here
(d8a199355f8f).
A stable backport that picks only this patch by its Fixes: tag would be
incomplete. Could the commit message mention the dependency?
> +
> /* 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 is a pre-existing issue, but what happens to usb_if when
dev_set_bus(dev, 0) fails while probing channel 0?
For channel 0, dev->prev_siblings is NULL, so
peak_usb_stop_rx(dev->prev_siblings) does not touch channel 0's own RX
URBs. pcan_usb_pro_free() then kfree()s usb_if because channel 0 has no
siblings. Only after that does the adap_dev_free path call
unregister_candev(netdev).
If userspace opened the netdev during probe, an RX completion in that
window would use the freed usb_if in pcan_usb_pro_decode_buf():
struct pcan_usb_pro_interface *usb_if = pcan_usb_pro_dev_if(dev);
Then unregister_candev() reaches pcan_usb_pro_stop() through ndo_stop,
which reads and decrements freed memory:
pdev->usb_if->dev_opened_count--;
It looks like the later commit in this series, "can: peak_usb:
unregister the netdev before dev_free() on probe failure", fixes this.
It calls unregister_candev() before dev_free(), so the final state of
the series is not affected.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v3-0-74d17c5a599e%40peak-system.fr
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 09/10] can: peak_usb: unregister the netdev before dev_free() on probe failure
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (7 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 08/10] can: peak_usb: reject records for unset channels in PCAN-USB Pro Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-21 22:58 ` netdev-bot+sashiko
2026-10-20 9:08 ` [PATCH v3 10/10] can: peak_usb: only dump received bytes on decode error Stéphane Grosjean
9 siblings, 1 reply; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, linux-can, linux-kernel
From: Stéphane Grosjean <s.grosjean@peak-system.fr>
When the device fails to switch the CAN bus off after the channel has
been initialized, peak_usb_create_dev() calls dev_free() before
unregister_candev(). For the first channel of PCAN-USB FD and Pro
adapters, dev_free() releases the interface object shared by all
channels, and also the command buffer on PCAN-USB FD.
The netdev is registered before the channel is initialized, so it may
have been opened in the meantime. unregister_candev() then closes it,
which accesses the released interface object and, on PCAN-USB FD,
sends commands through the released command buffer.
Unregister the netdev first, as peak_usb_disconnect() does.
Fixes: a0b96b4a6274 ("drivers: net: fix memory leak in peak_usb_create_dev")
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 5 +++++
1 file changed, 5 insertions(+)
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 4c08656e7768..981168fead92 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -1039,8 +1039,13 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
*/
peak_usb_stop_rx(dev->prev_siblings);
+ /* as in peak_usb_disconnect(), unregister the netdev before calling
+ * dev_free(): closing it may still use what dev_free() releases.
+ */
+ unregister_candev(netdev);
if (dev->adapter->dev_free)
dev->adapter->dev_free(dev);
+ goto lbl_restore_intf_data;
lbl_unregister_candev:
unregister_candev(netdev);
--
2.53.0
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 09/10] can: peak_usb: unregister the netdev before dev_free() on probe failure
2026-10-20 9:08 ` [PATCH v3 09/10] can: peak_usb: unregister the netdev before dev_free() on probe failure Stéphane Grosjean
@ 2026-10-21 22:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 16+ messages in thread
From: netdev-bot+sashiko @ 2026-10-21 22:58 UTC (permalink / raw)
To: stephane.grosjean
Cc: mkl, mailhol, error27, wg, s.grosjean, linux-can, linux-kernel, kuba
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 (2) · Medium: 0 (2) · Low: 0 (2)
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] Incomplete fix: the same close-after-free use-after-free is
still reachable when dev_init() fails on PCAN-USB FD channel 0.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
peak_usb_create_dev() makes the netdev visible with register_candev()…
- [High] This problem wasn't introduced by this patch, but the PCAN-USB
(pcan_usb.c) asynchronous restart is never drained before the device…
- [Medium] On PCAN-USB FD all channels share one command buffer
(pdev->cmd_buffer_addr is copied from channel 0 in pcan_usb_fd_init()).
- [Medium] If the registered netdev is opened between dev_init() and
probe's unconditional dev_set_bus(dev, 0) in peak_usb_create_dev(),…
- [Low] The comment above pcan_usb_pro_free() still says it is 'called
when PCAN-USB Pro adapter is unplugged'.
- [Low] On PCAN-USB FD and Pro, the first channel's dev_init() sends
drv_loaded(1) and sets the LED.
--
can: peak_usb: unregister the netdev before dev_free() on probe failure
This patch reorders the adap_dev_free error path in peak_usb_create_dev()
so that unregister_candev() runs before the adapter's dev_free() callback,
as peak_usb_disconnect() already does. The goal is to stop ndo_stop from
touching the shared interface object and the PCAN-USB FD command buffer
after dev_free() has released them when dev_set_bus(dev, 0) fails.
> 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 4c08656e77681..981168fead927 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -1039,8 +1039,13 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
> */
> peak_usb_stop_rx(dev->prev_siblings);
>
> + /* as in peak_usb_disconnect(), unregister the netdev before calling
> + * dev_free(): closing it may still use what dev_free() releases.
> + */
> + unregister_candev(netdev);
> if (dev->adapter->dev_free)
> dev->adapter->dev_free(dev);
[Severity: Low]
This is a pre-existing issue, but the comment above pcan_usb_pro_free() in
pcan_usb_pro.c still mentions only unplug:
/*
* called when PCAN-USB Pro adapter is unplugged
*/
static void pcan_usb_pro_free(struct peak_usb_device *dev)
This probe failure path also reaches it, and it can kfree() the shared
pcan_usb_pro_interface here. The FD version, pcan_usb_fd_free(), already
says "called when the USB adapter is unplugged, or when probing a sibling
fails".
Could the Pro comment be updated to match?
> + goto lbl_restore_intf_data;
[Severity: Low]
This isn't a bug introduced by this patch, but on PCAN-USB FD and Pro the
first channel's dev_init() has already told the firmware that the driver
is loaded:
pcan_usb_fd_init()
err = pcan_usb_fd_drv_loaded(dev, 1);
pcan_usb_pro_init()
err = pcan_usb_pro_drv_loaded(dev, 1);
Only dev_exit() sends the matching drv_loaded(0) and turns the LEDs off.
This path, the lbl_unregister_candev path and peak_usb_disconnect() never
call dev_exit().
After a failed probe, does the adapter stay connected in the "host driver
loaded" state with no driver bound?
>
> lbl_unregister_candev:
> unregister_candev(netdev);
[Severity: High]
This handles the dev_set_bus() failure. Can the same close-after-free
still happen when dev_init() fails on the first PCAN-USB FD channel?
That failure still ends up here:
peak_usb_create_dev() {
...
if (dev->adapter->dev_init) {
err = dev->adapter->dev_init(dev);
if (err)
goto lbl_unregister_candev;
}
...
}
On its error path, pcan_usb_fd_init() frees both shared objects and leaves
the pointers set:
pcan_usb_fd_init() {
...
err = pcan_usb_fd_drv_loaded(dev, 1);
if (err) {
...
goto err_out_2;
}
...
err_out_2:
kfree(pdev->cmd_buffer_addr);
err_out_1:
kfree(pdev->usb_if);
err_out:
return err;
}
The netdev is registered before dev_init() runs, and the device can stall
the pcan_usb_fd_drv_loaded() request until it times out. If the interface
is brought up during that window, pcan_usb_fd_start() increments
usb_if->dev_opened_count. After the free, unregister_candev() here then
runs:
unregister_candev()
peak_usb_ndo_stop()
peak_usb_unlink_all_urbs()
pcan_usb_fd_stop()
if (pdev->usb_if->dev_opened_count == 1)
pcan_usb_fd_set_options() <- command built in freed cmd buffer
pdev->usb_if->dev_opened_count--;
pcan_usb_fd_set_bus(dev, 0) <- freed cmd buffer written again
pcan_usb_fd_send_cmd() checks PCAN_USB_STATE_CONNECTED, but that bit is
still set for dev on this path, so the sends go ahead. The RX URBs
submitted by the open also keep decoding through the freed usb_if until
peak_usb_unlink_all_urbs() kills them.
The commit message describes this same scenario, and no later patch in
the series touches this path.
Would it be better to keep dev_init() from freeing state the netdev can
still reach? Another option is to register the netdev only after
dev_init() and dev_set_bus(dev, 0) succeed.
[Severity: High]
This isn't a bug introduced by this patch, but register_candev() runs
before dev_init() and dev_set_bus(dev, 0). What stops an open from racing
with the half-initialized device? The only check in peak_usb_ndo_open() is:
if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
return -ENODEV;
That bit is already set before registration.
On PCAN-USB Pro, pcan_usb_pro_init() sets pdev->usb_if only at the very
end. Before that it sends two pcan_usb_pro_send_req() INFO requests and
calls pcan_usb_pro_drv_loaded(), and the device can stall each of them:
pcan_usb_pro_init() {
...
pdev->usb_if = usb_if;
...
}
Suppose the bitrate is set and the interface brought up during that
window:
peak_usb_ndo_open()
peak_usb_start()
pcan_usb_pro_start()
if (pdev->usb_if->dev_opened_count == 0) <- usb_if still NULL
Would this be a NULL pointer oops?
FD has the same problem with a shorter window. pcan_usb_fd_set_bittiming_slow(),
pcan_usb_fd_start() and pcan_usb_fd_send_cmd() dereference
pcan_usb_fd_cmd_buffer(dev) and pdev->usb_if before pcan_usb_fd_init()
assigns them.
[Severity: Medium]
This is also pre-existing and comes from the same register-before-init
ordering. Suppose the netdev is opened after dev_init() but before this
probe-time call in peak_usb_create_dev():
/* set bus off */
if (dev->adapter->dev_set_bus) {
err = dev->adapter->dev_set_bus(dev, 0);
This puts the controller back in reset mode (PUCAN_CMD_RESET_MODE on FD,
SETBUSACT 0 on Pro). If the command succeeds, probe returns 0 and the
netdev stays up: can.state is CAN_STATE_ERROR_ACTIVE,
PCAN_USB_STATE_STARTED is set and the TX queue is running.
Does that leave an interface that neither sends nor receives until it is
taken down and brought up again?
[Severity: Medium]
This is a pre-existing issue, but on PCAN-USB FD every channel shares one
command buffer, copied from channel 0 in pcan_usb_fd_init():
pdev->cmd_buffer_addr = ppdev->cmd_buffer_addr;
While probe initializes channel N, it writes into that buffer without RTNL
or any other lock. The writes come from pcan_usb_fd_set_clock_domain() and
pcan_usb_fd_set_can_led(), and then from pcan_usb_fd_set_bus(dev, 0) in
peak_usb_create_dev().
Meanwhile, a sibling that is already registered can be opened, configured
or driven through ethtool under RTNL, and it writes the same buffer, for
example:
pcan_usb_fd_start()
pcan_usb_fd_set_filter_std()
Can these commands interleave, so that the adapter receives a corrupted
command list?
[Severity: High]
This isn't a bug introduced by this patch either. On PCAN-USB (pcan_usb.c),
the asynchronous restart doesn't seem to be drained before free_candev(),
either on this error path or in peak_usb_disconnect().
pcan_usb_restart_async() submits an unanchored URB with pdev as its
context:
usb_fill_bulk_urb(urb, dev->udev,
usb_sndbulkpipe(dev->udev, PCAN_USB_EP_CMDOUT),
buf, PCAN_USB_CMD_LEN,
pcan_usb_restart_pending, pdev);
return usb_submit_urb(urb, GFP_ATOMIC);
The completion arms a timer without checking urb->status:
pcan_usb_restart_pending()
mod_timer(&pdev->restart_timer,
jiffies + msecs_to_jiffies(PCAN_USB_STARTUP_TIMEOUT));
The timer callback then touches the device:
pcan_usb_restart()
peak_usb_restart_complete(dev)
dev->can.state = CAN_STATE_ERROR_ACTIVE;
netif_wake_queue(dev->netdev);
Nothing in pcan_usb.c calls timer_delete() or timer_shutdown() on
restart_timer, and the adapter has no dev_stop or dev_free callback.
peak_usb_ndo_stop() only kills the RX and TX anchors, and close_candev()
only cancels the CAN core restart work.
Suppose a bus-off restart (restart-ms, or "ip link set canX type can
restart") is still in flight when the device is unplugged or probe fails.
Can the URB completion or the timer then run on the netdev private area
after free_candev() has freed it?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v3-0-74d17c5a599e%40peak-system.fr
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 10/10] can: peak_usb: only dump received bytes on decode error
2026-10-20 9:08 [PATCH v3 00/10] can: peak_usb: fixes, cleanup and input validation hardening Stéphane Grosjean
` (8 preceding siblings ...)
2026-10-20 9:08 ` [PATCH v3 09/10] can: peak_usb: unregister the netdev before dev_free() on probe failure Stéphane Grosjean
@ 2026-10-20 9:08 ` Stéphane Grosjean
2026-10-21 22:58 ` netdev-bot+sashiko
9 siblings, 1 reply; 16+ messages in thread
From: Stéphane Grosjean @ 2026-10-20 9:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Dan Carpenter, Wolfgang Grandegger
Cc: Stéphane Grosjean, 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.
Dump only the urb->actual_length bytes received from the device. Since a
faulty or malicious device can make every transfer fail to decode, also
rate limit the dump.
Fixes: bb4785551f64 ("can: usb: PEAK-System Technik USB adapters driver core")
Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 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 981168fead92..a58317cdde4e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -265,10 +265,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);
}
}
--
2.53.0
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 10/10] can: peak_usb: only dump received bytes on decode error
2026-10-20 9:08 ` [PATCH v3 10/10] can: peak_usb: only dump received bytes on decode error Stéphane Grosjean
@ 2026-10-21 22:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 16+ messages in thread
From: netdev-bot+sashiko @ 2026-10-21 22:58 UTC (permalink / raw)
To: stephane.grosjean
Cc: mkl, mailhol, error27, wg, s.grosjean, linux-can, linux-kernel, kuba
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (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).
- [High] The patch only partly does what it says.
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
pcan_usb_pro_decode_buf() (pcan_usb_pro.c:737-758) runs its loop on…
--
can: peak_usb: only dump received bytes on decode error
When decoding fails, peak_usb_read_bulk_callback() now dumps only the
urb->actual_length bytes received from the device, not the whole RX
buffer. That dump is also rate limited with net_ratelimit(), because a
faulty or malicious device can make every transfer fail to decode.
> 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 981168fead927..a58317cdde4eb 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -265,10 +265,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);
[Severity: High]
Does this rate limit cover the case the commit message describes? The FD
and Pro decoders dump the same buffer themselves, with no rate limit,
before they return the error to this callback:
drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_decode_buf() {
...
fail:
if (err)
pcan_dump_mem("received msg",
urb->transfer_buffer, urb->actual_length);
return err;
}
drivers/net/can/usb/peak_usb/pcan_usb_pro.c:pcan_usb_pro_decode_buf() {
...
fail:
if (err)
pcan_dump_mem("received msg",
urb->transfer_buffer, urb->actual_length);
return err;
}
Device-controlled data reaches these paths: "got frag rec", "got short
rec", "got unsupported rec", and errors from the per-record handlers. The
URB is resubmitted right away.
Can a faulty or malicious device that presents as an FD-family or Pro
adapter still flood the log on every bulk-IN completion? Each dump can be
up to rx_buffer_size bytes, which is 2048 for FD and 1024 for Pro.
As far as I can tell, only pcan_usb_decode_buf() gets the benefit of the
net_ratelimit() here, since it has no dump of its own. For FD and Pro
adapters, whenever net_ratelimit() allows it, the same bytes are printed
twice.
The netdev_err() messages on those failure paths aren't rate limited
either. One example is in pcan_usb_pro_decode_buf():
if (!rec_ptr) {
netdev_err(netdev, "bad msg hdr len %d\n", urb->actual_length);
return -EINVAL;
}
Another is the "usb message length error" message in pcan_usb.c.
One option is to drop the decoder-level dumps, so this rate-limited dump
is the only one. Another is to put them behind net_ratelimit() too. In
either case, the related netdev_err() calls could be guarded the same
way.
[Severity: Medium]
This is a pre-existing issue and wasn't introduced by this patch. While
looking at the decoder dumps, though, is there an out-of-bounds read in
pcan_usb_pro_decode_buf()? The loop count comes from the device, and
pr->data_type is read before rec_ptr is checked against msg_end:
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];
...
/* check if the record goes out of current packet */
if (rec_ptr + sizeof_rec > msg_end) {
Suppose the records exactly fill urb->actual_length and rec_cnt claims
more. Then rec_ptr ends up equal to msg_end, and the next iteration reads
transfer_buffer[actual_length].
If actual_length is PCAN_USBPRO_RX_BUFFER_SIZE (1024), that byte is one
past the kmalloc() buffer. KASAN would report it as slab-out-of-bounds.
pcan_usb_pro_wait_rsp() has the same pattern, and it has no
end-of-buffer check at all:
rec_cnt = le32_to_cpu(*rsp.u.rec_cnt);
/* loop on records stored into message */
for (r = 0; r < rec_cnt; r++) {
pr = (union pcan_usb_pro_rec *)pc;
rec_len = pcan_usb_pro_sizeof_rec[pr->data_type];
With a u32 rec_cnt supplied by the device, can pc walk past
actual_length here? If so, the out-of-bounds bytes could be printed by
the "got unwanted rsp %xh" and "got rsp %xh but on chan%u" messages.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v3-0-74d17c5a599e%40peak-system.fr
^ permalink raw reply [flat|nested] 16+ messages in thread