[PATCH v2 0/5] can: peak_usb: fixes, cleanup and input validation hardening

[PATCH v2 0/5] can: peak_usb: fixes, cleanup and input validation hardening

[Stéphane Grosjean]

This series contains a mix of bug fixes, cleanups, and hardening
improvements for the PEAK-System USB CAN drivers.

The first patch fixes a bug in CAN error frame generation where
CAN_ERR_CNT accidentally overwrites previously recorded CAN error
flags. As a result, information about previously detected error
conditions may be lost when error counters are reported.

The second and third patches are cleanup changes. One makes a public
header self-contained by including all required dependencies, while
the other reorders CAN_CTRLMODE_* flags in ctrlmode_supported for
improved readability without changing driver behavior.

The remaining patches strengthen validation of data received from USB
devices. They verify device-provided CAN channel numbers before they
are used as array indexes and introduce additional bounds checking
when parsing received message buffers.

Malformed messages are now rejected consistently, preventing possible
out-of-bounds memory accesses caused by corrupted or otherwise
untrusted device input.

---
Changes in v2:
- Kill all pending RX URBs before unregistering sibling netdevs.
- Reject records targeting unavailable channels during device
  initialization in rare corner cases involving malformed device
  input.
- Link to v1: https://patch.msgid.link/20261008-canfd_check_channel_idx-v1-0-0a3bb82f4e09@peak-system.fr

To: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol@kernel.org>
To: Dan Carpenter <error27@gmail.com>
To: Wolfgang Grandegger <wg@grandegger.com>
Cc: Stéphane Grosjean <s.grosjean@peak-system.fr>
Cc: linux-can@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Marc Kleine-Budde (1):
      can: peak_usb: add missing includes

Stefan Günther (1):
      can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters

Stéphane Grosjean (3):
      can: peak_usb: sort ctrlmode_supported flags
      can: peak_usb: validate channel numbers in PCAN-USB FD
      can: peak_usb: harden PCAN-USB message validation

 drivers/net/can/usb/peak_usb/pcan_usb.c      | 29 ++++++----
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 12 +++-
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  6 ++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 84 ++++++++++++++++++++++------
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  |  3 +-
 5 files changed, 104 insertions(+), 30 deletions(-)
---
base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7
change-id: 20261007-canfd_check_channel_idx-a4d7a1a1ee7d

Best regards,
--  
Stéphane Grosjean <s.grosjean@peak-system.fr>

[PATCH v2 1/5] can: peak_usb: fix missing CAN_ERR_FLAG when reporting error counters

[Stéphane Grosjean]

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.43.0

[PATCH v2 2/5] can: peak_usb: add missing includes

[Stéphane Grosjean]

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.43.0

[PATCH v2 3/5] can: peak_usb: sort ctrlmode_supported flags

[Stéphane Grosjean]

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.43.0

[PATCH v2 4/5] can: peak_usb: validate channel numbers in PCAN-USB FD

[Stéphane Grosjean]

From: Stéphane Grosjean <s.grosjean@peak-system.fr>

The PCAN-USB FD family encodes the CAN channel number in messages
received from the device. This value is used as an index into the
adapter CAN device table.

Validate the channel number against the number of CAN controllers
supported by the adapter before performing the lookup. Any message
containing an invalid channel number is treated as malformed and the
entire message buffer is discarded, as the device is considered to be
providing untrusted data.

Additionally, return -EINVAL instead of -ENOMEM when an invalid
channel number is detected, making the error reporting consistent
with the rest of the driver.

This prevents potential out-of-bounds accesses when handling
unexpected or corrupted messages received from PCAN-USB FD family
devices.

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 | 12 ++++++-
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 52 ++++++++++++++++++++++++----
 2 files changed, 57 insertions(+), 7 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 55aad01cd8ca..751cd52cb548 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -1038,7 +1038,17 @@ 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 */
+	/* First, kill all pending RX URBs. usb_kill_anchored_urbs() waits
+	 * until all completion handlers have completed, ensuring that no
+	 * decode_buf() callback can access usb_if->dev[] after this point.
+	 */
+	for (dev = usb_get_intfdata(intf); dev; dev = dev->prev_siblings)
+		usb_kill_anchored_urbs(&dev->rx_submitted);
+
+	/* All RX URBs have been drained before reaching this point. No
+	 * decode_buf() callback can access usb_if->dev[] anymore, making it
+	 * safe to unregister and free the associated netdevs.
+	 */
 	for (dev = usb_get_intfdata(intf); dev; dev = dev_prev_siblings) {
 		struct net_device *netdev = dev->netdev;
 		char name[IFNAMSIZ];
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..71ac6fdcd99b 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,19 @@ 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 = usb_if->dev[pucan_msg_get_channel(rm)];
+
+	/* This should never happen during normal operation. However, do not
+	 * trust the device and reject records targeting a valid channel
+	 * without an associated netdev.
+	 */
+	if (!dev)
+		return -EINVAL;
+
 	netdev = dev->netdev;
 
 	if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {
@@ -605,10 +615,19 @@ 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 = usb_if->dev[pucan_stmsg_get_channel(sm)];
+
+	/* This should never happen during normal operation. However, do not
+	 * trust the device and reject records targeting a valid channel
+	 * without an associated netdev.
+	 */
+	if (!dev)
+		return -EINVAL;
+
 	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
 	netdev = dev->netdev;
 
@@ -662,10 +681,19 @@ 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 = usb_if->dev[pucan_ermsg_get_channel(er)];
+
+	/* This should never happen during normal operation. However, do not
+	 * trust the device and reject records targeting a valid channel
+	 * without an associated netdev.
+	 */
+	if (!dev)
+		return -EINVAL;
+
 	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
 
 	/* keep a trace of tx and rx error counters for later use */
@@ -685,10 +713,19 @@ 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 = usb_if->dev[pufd_omsg_get_channel(ov)];
+
+	/* This should never happen during normal operation. However, do not
+	 * trust the device and reject records targeting a valid channel
+	 * without an associated netdev.
+	 */
+	if (!dev)
+		return -EINVAL;
+
 	netdev = dev->netdev;
 
 	/* allocate an skb to store the error frame */
@@ -987,6 +1024,9 @@ static int pcan_usb_fd_init(struct peak_usb_device *dev)
 		if (!pdev->cmd_buffer_addr)
 			goto err_out_1;
 
+		/* keep reference to the adapter device */
+		pdev->usb_if->adapter = dev->adapter;
+
 		/* number of ts msgs to ignore before taking one into account */
 		pdev->usb_if->cm_ignore_count = 5;
 

-- 
2.43.0

[PATCH v2 5/5] can: peak_usb: harden PCAN-USB message validation

[Stéphane Grosjean]

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 | 24 ++++++++++++++++--------
 1 file changed, 16 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..56ddd134bae0 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -696,8 +696,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 +736,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.43.0

Re: [PATCH v2 4/5] can: peak_usb: validate channel numbers in PCAN-USB FD

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (2) · Medium: 2 (2) · Low: 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] The new `if (!dev) return -EINVAL;` guards in the four FD
  decoders are commented as rejecting "records targeting a valid channel…
- [Medium] The patch makes a change to the shared core teardown path that
  its commit message does not mention.
- [Medium] The new comments in peak_usb_disconnect() claim that after the
  usb_kill_anchored_urbs() loop "No decode_buf() callback can access…
- [Low] The commit message says the patch "prevents potential out-of-
  bounds accesses".
- [Low] The new `if (!dev)` checks in
  pcan_usb_fd_decode_canmsg/status/error/overrun exist for the case…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the sibling
  PCAN-USB Pro driver has the same NULL-slot dereference that the patch…
- [High] This problem wasn't introduced by this patch, but the error path
  in peak_usb_create_dev() frees the shared FD interface object while…
- [Medium] This problem wasn't introduced by this patch, but
  peak_usb_create_dev() calls register_candev(netdev)…
- [Medium] This problem wasn't introduced by this patch, but when a
  decoder returns an error, peak_usb_read_bulk_callback()…

--

can: peak_usb: validate channel numbers in PCAN-USB FD

The patch replaces the ARRAY_SIZE(usb_if->dev) checks in the four
PCAN-USB FD decoders with a check against the adapter ctrl_count. It also
adds a NULL check on the usb_if->dev[] slot and returns -EINVAL instead
of -ENOMEM. Finally, it adds a loop to peak_usb_disconnect() that kills
all anchored RX URBs before any netdev is unregistered.

> This prevents potential out-of-bounds accesses when handling
> unexpected or corrupted messages received from PCAN-USB FD family
> devices.

[Severity: Low]
Is "out-of-bounds accesses" the right description? The commit in the
Fixes: tag (a6921dd524fe) already added ARRAY_SIZE(usb_if->dev) checks to
all four decoders, so the index could not go past the array.

ARRAY_SIZE(usb_if->dev) is PCAN_USB_MAX_CHANNEL, which is 2. The
pcan_usb_fd and pcan_usb_chip adapters use ctrl_count =
PCAN_USBFD_CHANNEL_COUNT, which is 1.

On those adapters a record with channel 1 passed the old check. It then
reached usb_if->dev[1], which is never assigned and so stays NULL from the
kzalloc. That NULL was dereferenced through dev->netdev or
container_of(dev, ...)->bec.

Would it be more accurate to describe this as a NULL pointer dereference?

> 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 55aad01cd8cac..751cd52cb5486 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -1038,7 +1038,17 @@ 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 */
> +	/* First, kill all pending RX URBs. usb_kill_anchored_urbs() waits
> +	 * until all completion handlers have completed, ensuring that no
> +	 * decode_buf() callback can access usb_if->dev[] after this point.
> +	 */
> +	for (dev = usb_get_intfdata(intf); dev; dev = dev->prev_siblings)
> +		usb_kill_anchored_urbs(&dev->rx_submitted);

[Severity: Medium]
The commit message only covers channel number validation in the PCAN-USB
FD decoders. Should this change to peak_usb_disconnect() go in its own
patch, or at least be described here?

peak_usb_disconnect() is the .disconnect callback of peak_usb_driver, so
this changes teardown for PCAN-USB, PCAN-USB Pro and all FD variants. The
Fixes: tag points at the range-check commit, which did not touch teardown
ordering.

It is also unclear which case the loop fixes. peak_usb_driver does not set
soft_unbind, so on a normal unbind the USB core already does this:

drivers/usb/core/driver.c:usb_unbind_interface() {
    ...
	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
		usb_disable_interface(udev, intf, false);

	driver->disconnect(intf);
    ...
}

That call flushes all URBs on the interface before ->disconnect runs. As
far as I can tell, the new loop only changes behaviour when
peak_usb_probe() calls peak_usb_disconnect() directly after
peak_usb_create_dev() fails:

peak_usb_probe() {
    ...
		err = peak_usb_create_dev(peak_usb_adapter, intf, i);
		if (err) {
			/* deregister already created devices */
			peak_usb_disconnect(intf);
			break;
		}
    ...
}

> +
> +	/* All RX URBs have been drained before reaching this point. No
> +	 * decode_buf() callback can access usb_if->dev[] anymore, making it
> +	 * safe to unregister and free the associated netdevs.
> +	 */

[Severity: Medium]
Is this guarantee actually enforced? usb_kill_anchored_urbs() only blocks
resubmission while it is running.

The netdevs stay registered. PCAN_USB_STATE_CONNECTED is only cleared per
device, inside the second loop, and no lock is held across the two loops.
So a concurrent open can re-arm the RX URBs after the drain:

peak_usb_ndo_open()
  peak_usb_start()
    usb_submit_urb()

On a normal unbind those submissions fail, because the interface has
already been disabled. On the peak_usb_probe() failure path the endpoints
are still enabled.

Take a two-channel FD adapter where creating channel 1 failed after
dev_init. usb_if->dev[1] still points at the freed channel 1 device (see
the comment in pcan_usb_fd_decode_canmsg() below).

Suppose can0 is brought up between the first loop and
unregister_candev(can0). Could a channel=1 record then be decoded through
the freed device?

>  	for (dev = usb_get_intfdata(intf); dev; dev = dev_prev_siblings) {
>  		struct net_device *netdev = dev->netdev;
>  		char name[IFNAMSIZ];
> 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 82502594a4092..71ac6fdcd99be 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,19 @@ 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;

[Severity: Medium]
This isn't a bug introduced by this patch, but when a decoder returns an
error like this one, peak_usb_read_bulk_callback() dumps the whole RX
buffer:

peak_usb_read_bulk_callback() {
    ...
			err = dev->adapter->dev_decode_buf(dev, urb);
			if (err)
				pcan_dump_mem("received usb message",
					      urb->transfer_buffer,
					      urb->transfer_buffer_length);
    ...
}

transfer_buffer_length is the allocated size, PCAN_UFD_RX_BUFFER_SIZE
(2048 bytes) for FD, not urb->actual_length. peak_usb_start() allocates
these buffers with a plain kmalloc().

Can a device send a short transfer with a bad channel number on the first
completion of each URB? If so, stale heap contents past actual_length
would go to the kernel log through print_hex_dump(KERN_INFO, ...).

The dump is not rate limited, and every ndo_open allocates new buffers.

Would dumping only urb->actual_length bytes be enough?

>  
>  	dev = usb_if->dev[pucan_msg_get_channel(rm)];
> +
> +	/* This should never happen during normal operation. However, do not
> +	 * trust the device and reject records targeting a valid channel
> +	 * without an associated netdev.
> +	 */
> +	if (!dev)
> +		return -EINVAL;
> +
>  	netdev = dev->netdev;

[Severity: High]
Does this check cover a channel whose device has already been freed?
Nothing ever clears usb_if->dev[]. The only writer is in
pcan_usb_fd_init():

	pdev->usb_if->dev[dev->ctrl_idx] = dev;

pcan_usb_fd_free() does nothing for a channel that still has siblings:

	if (!dev->prev_siblings && !dev->next_siblings) {

Consider probe of a two-channel FD adapter such as PCAN-USB Pro FD or X6:

peak_usb_create_dev(0)              can0 brought up, RX URBs live
peak_usb_create_dev(1)
  register_candev(can1)
  pcan_usb_fd_init()                dev[1] = dev1
  dev_set_bus(dev1, 0) fails        device stalls or times out
  adap_dev_free:
    pcan_usb_fd_free(dev1)          no-op, prev_siblings is set
  unregister_candev(can1)
  free_candev(can1)                 dev1 freed, dev[1] still set

Until peak_usb_probe() reaches peak_usb_disconnect(), a channel=1 record
on one of can0's URBs passes both the ctrl_count check and the !dev
check.

pcan_usb_fd_decode_error() then writes device-supplied counters into the
freed object:

	pdev->bec.txerr = er->tx_err_cnt;
	pdev->bec.rxerr = er->rx_err_cnt;

pcan_usb_fd_decode_canmsg(), pcan_usb_fd_decode_status() and
pcan_usb_fd_decode_overrun() read dev->netdev from freed memory and use
it.

Should the slot be cleared, and sibling RX quiesced, before a channel that
was already published to usb_if->dev[] is freed?

[ ... ]

> @@ -605,10 +615,19 @@ 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 = usb_if->dev[pucan_stmsg_get_channel(sm)];
> +
> +	/* This should never happen during normal operation. However, do not
> +	 * trust the device and reject records targeting a valid channel
> +	 * without an associated netdev.
> +	 */
> +	if (!dev)
> +		return -EINVAL;
> +
>  	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
>  	netdev = dev->netdev;

[Severity: Low]
The new NULL checks in pcan_usb_fd_decode_canmsg(),
pcan_usb_fd_decode_status(), pcan_usb_fd_decode_error() and
pcan_usb_fd_decode_overrun() target a sibling's RX completion that runs
while probe is publishing the slot in pcan_usb_fd_init().

Both sides use plain accesses: a plain store in process context and a
plain load in URB completion context. KCSAN would report this as a data
race.

Nothing orders the ctrlmode and ctrlmode_supported copies made in the
sibling branch of pcan_usb_fd_init() before the store either.
pcan_usb_fd_decode_canmsg() reads dev->can.ctrlmode.

The slot only ever goes from NULL to non-NULL, so the impact looks
limited. Should the store and loads use WRITE_ONCE()/smp_store_release()
and READ_ONCE()/smp_load_acquire()?

[ ... ]

> @@ -987,6 +1024,9 @@ static int pcan_usb_fd_init(struct peak_usb_device *dev)
>  		if (!pdev->cmd_buffer_addr)
>  			goto err_out_1;
>  
> +		/* keep reference to the adapter device */
> +		pdev->usb_if->adapter = dev->adapter;
> +
>  		/* number of ts msgs to ignore before taking one into account */
>  		pdev->usb_if->cm_ignore_count = 5;

[Severity: Medium]
This is a pre-existing issue, but peak_usb_create_dev() calls
register_candev() before dev->adapter->dev_init(). The FD netdev is
therefore visible to userspace before pcan_usb_fd_init() has set
pdev->usb_if, pdev->cmd_buffer_addr and usb_if->dev[].

A bitrate change in that window goes through peak_usb_set_bittiming() to
pcan_usb_fd_set_bittiming_slow(). That function writes through
pcan_usb_fd_cmd_buffer(dev) while it is still NULL. pcan_usb_fd_send_cmd()
also dereferences pdev->usb_if->fw_info.

This patch adds a small sub-window of its own. usb_if is kzalloc'ed and
published, and cmd_buffer_addr is allocated, before usb_if->adapter is
assigned here. Every decoder now starts by dereferencing
usb_if->adapter->ctrl_count.

That sub-window is very hard to hit, because decoding requires
PCAN_USB_STATE_STARTED. Before the patch, the same window crashed on the
NULL dev[] slot instead.

Could usb_if->adapter be assigned right after the kzalloc? The root fix
would be to finish dev_init before register_candev().

[ ... ]

[Severity: High]
This is a pre-existing issue, but it is in the same probe teardown code.
The error path in peak_usb_create_dev() frees the shared FD state before
the netdev is unregistered:

peak_usb_create_dev() {
    ...
adap_dev_free:
	if (dev->adapter->dev_free)
		dev->adapter->dev_free(dev);

lbl_unregister_candev:
	unregister_candev(netdev);
    ...
}

For channel 0 of any FD adapter, pcan_usb_fd_free() sees no siblings. It
kfree()s pdev->cmd_buffer_addr and pdev->usb_if and does not clear either
pointer.

If can0 was opened between register_candev() and the failing
dev_set_bus(dev, 0), unregister_candev() closes it:

unregister_candev()
  peak_usb_ndo_stop()
    pcan_usb_fd_stop()
      pcan_usb_fd_set_options()          writes freed cmd_buffer_addr
      pdev->usb_if->dev_opened_count--   writes freed usb_if
    pcan_usb_fd_set_bus()                writes freed cmd_buffer_addr

pcan_usb_fd_send_cmd() also reads fw_info from the freed usb_if. Until
can0 is closed, its RX URBs keep decoding through the freed usb_if.

Should unregister_candev() run before dev_free here, in the same order
peak_usb_disconnect() uses?

[Severity: High]
This isn't a bug introduced by this patch, but the PCAN-USB Pro driver has
the same NULL slot dereference that this patch guards against in the FD
decoders. pcan_usb_pro_handle_canmsg() and pcan_usb_pro_handle_error() in
pcan_usb_pro.c only do this:

	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
		return -EINVAL;

	dev = usb_if->dev[ctrl_idx];
	netdev = dev->netdev;

pcan_usb_pro_init() sets usb_if->dev[1] only when channel 1 is created. By
then can0 is already registered and can be opened, and its RX URBs are
live on the shared PCAN_USBPRO_EP_MSGIN endpoint.

The device can widen that window by delaying its responses to can0's
dev_set_bus(0) and dev_get_can_channel_id calls.

Can a record with ctrl_idx == 1 arriving in that window cause a NULL
pointer dereference in URB completion context? Should the Pro decoders get
the same NULL check?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-canfd_check_channel_idx-v2-0-f7b772929791%40peak-system.fr

Re: [PATCH v2 5/5] can: peak_usb: harden PCAN-USB message validation

[netdev-bot+sashiko]

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 patch says it rejects PCAN-USB messages that would cause
  reads outside the received USB data, but one path is still unchecked:…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the encoder
  and decoder disagree on the wire layout for RTR frames with SRR set.
- [Medium] This problem wasn't introduced by this patch, but when the
  decoder rejects a message, the diagnostic dump prints the whole rx…

--

can: peak_usb: harden PCAN-USB message validation

This patch adds bounds checks to the PCAN-USB rx decoder. The status_len
byte at the start of each record and the SRR private-id byte after a data
frame are now consumed only if they fit inside the received USB buffer.
Malformed messages are rejected with -EINVAL.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 169c00d93463c..56ddd134bae0f 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -696,8 +696,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++;
> +		}

[Severity: Medium]
This is a pre-existing issue, but is the private-id byte also present for
RTR frames?

In pcan_usb_decode_data(), this skip and the new check exist only in the
non-RTR branch:

	if (status_len & PCAN_USB_STATUSLEN_RTR) {
		cf->can_id |= CAN_RTR_FLAG;
	} else {
		...
		if (can_id_flags & PCAN_USB_TX_SRR) {

But pcan_usb_encode_msg() appends the writer id for RTR frames as well:

	/* can data */
	if (!(cf->can_id & CAN_RTR_FLAG)) {
		memcpy(pc, cf->data, cf->len);
		pc += cf->len;
	}

	/* SRR bit needs a writer id (useless here) */
	if (can_id_flags & PCAN_USB_TX_SRR)
		*pc++ = 0x80;

SRR is set whenever CAN_CTRLMODE_LOOPBACK is enabled. Suppose the firmware's
loopback echo of an RTR frame includes that byte. Then mc->ptr is left
pointing at it, and pcan_usb_decode_msg() reads 0x80 as the next status_len
byte.

PCAN_USB_STATUSLEN_INTERNAL is clear in 0x80, so the byte would be decoded
as a bogus standard-ID data frame. Every later record in the URB would then
be parsed out of phase. That gives phantom frames, or an early -EINVAL that
drops the rest of the URB.

The real RTR echo format can't be confirmed from the source. Both the
encoder and decoder lines came from the same commit 3a7939495ce8, so the
difference may be intentional. The later reads are bounds-checked, so no
out-of-bounds access results.

Could the firmware behaviour be checked either way?

> @@ -733,14 +736,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;

[Severity: Medium]
This isn't a bug introduced by this patch, but this new -EINVAL path (and
the truncated SRR one above) leads to the diagnostic dump in
peak_usb_read_bulk_callback():

	err = dev->adapter->dev_decode_buf(dev, urb);
	if (err)
		pcan_dump_mem("received usb message",
			      urb->transfer_buffer,
			      urb->transfer_buffer_length);

Doesn't this dump the full rx_buffer_size allocation instead of only the
urb->actual_length bytes the device sent?

The rx buffers come from kmalloc() in peak_usb_start() and are not zeroed.
After a short malformed transfer such as [00 01 00] (truncated CAN id), the
bytes past the received data are uninitialized slab contents.

pcan_dump_mem() prints them at KERN_INFO with print_hex_dump(), with no rate
limit and no debug-only guard. Earlier decode failures could already reach
this dump.

Would it make sense to pass urb->actual_length here instead?

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

[Severity: High]
Can a truncated bus event record still cause an out-of-bounds read here?

pcan_usb_decode_status() only checks that the function and number bytes
fit, and then advances past them:

	if ((mc->ptr + 2) > mc->end)
		return -EINVAL;

	f = mc->ptr[PCAN_USB_CMD_FUNC];
	n = mc->ptr[PCAN_USB_CMD_NUM];
	mc->ptr += PCAN_USB_CMD_ARGS;

At this point mc->ptr can equal mc->end. Suppose the timestamp bit is clear,
f is PCAN_USB_REC_BUSEVT, and n is PCAN_USB_ERR_CNT_DEC or
PCAN_USB_ERR_CNT_INC. Then the call goes straight to
pcan_usb_handle_bus_evt():

		pdev->bec.rxerr = mc->ptr[1];
		pdev->bec.txerr = mc->ptr[2];

These read mc->end + 1 and mc->end + 2 with no check. The later
(mc->ptr + rec_len) > mc->end check in pcan_usb_decode_status() runs only
after these reads. rec_len also comes from the device and can be 0.

For example, the 5 byte transfer [00 01 40 05 00] passes the new status_len
check. It then reads ibuf[6] and ibuf[7], past actual_length, and is
reported as successfully decoded.

Now take a full 64 byte transfer whose last record is a bus event header
ending at the last byte. The reads land on ibuf[65] and ibuf[66], which are
outside the kmalloc-64 rx buffer allocated in peak_usb_start().

Those bytes are stored in pdev->bec. Userspace can read them through
pcan_usb_get_berr_counter() (IFLA_CAN_BERR_COUNTER) and in the CAN error
frames built by pcan_usb_decode_error().

The commit message says the patch rejects buffers "whose contents would
otherwise lead to accesses outside the received USB data area". Should
pcan_usb_handle_bus_evt() check something like (mc->ptr + 3) > mc->end
before reading these bytes, rather than relying on rec_len from the device?

>  			/* handle normal can frames here */
> +			} else {
> +				err = pcan_usb_decode_data(&mc, sl);
> +			}
>  		}
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-canfd_check_channel_idx-v2-0-f7b772929791%40peak-system.fr