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

[PATCH 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.

Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
---
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.h |  6 +++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 64 ++++++++++++++++++++--------
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  |  3 +-
 4 files changed, 73 insertions(+), 29 deletions(-)
---
base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7
change-id: 20261007-canfd_check_channel_idx-a4d7a1a1ee7d

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

[PATCH 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 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 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 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_fd.c | 32 ++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

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..ddbb2be1de0d 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,8 +537,12 @@ 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.
+	 * Any channel below ctrl_count is guaranteed to have a valid
+	 * entry in usb_if->dev[].
+	 */
+	if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
+		return -EINVAL;
 
 	dev = usb_if->dev[pucan_msg_get_channel(rm)];
 	netdev = dev->netdev;
@@ -605,8 +610,12 @@ 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.
+	 * Any channel below ctrl_count is guaranteed to have a valid
+	 * entry in usb_if->dev[].
+	 */
+	if (pucan_stmsg_get_channel(sm) >= usb_if->adapter->ctrl_count)
+		return -EINVAL;
 
 	dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
 	pdev = container_of(dev, struct pcan_usb_fd_device, dev);
@@ -662,7 +671,11 @@ 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.
+	 * Any channel below ctrl_count is guaranteed to have a valid
+	 * entry in usb_if->dev[].
+	 */
+	if (pucan_ermsg_get_channel(er) >= usb_if->adapter->ctrl_count)
 		return -EINVAL;
 
 	dev = usb_if->dev[pucan_ermsg_get_channel(er)];
@@ -685,7 +698,11 @@ 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.
+	 * Any channel below ctrl_count is guaranteed to have a valid
+	 * entry in usb_if->dev[].
+	 */
+	if (pufd_omsg_get_channel(ov) >= usb_if->adapter->ctrl_count)
 		return -EINVAL;
 
 	dev = usb_if->dev[pufd_omsg_get_channel(ov)];
@@ -987,6 +1004,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: peak_usb: missing NULL check on sibling channels leads to UAF and NULL deref
--

commit bb6a3841806ddddd6a5d411fd8594a762ea452bd
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

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

This patch adds validation for CAN channel numbers encoded in messages
received from PCAN-USB FD devices. The channel number is checked against
the number of controllers supported by the adapter to prevent out-of-bounds
array accesses.

> 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..ddbb2be1de0dd 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
[ ... ]
> @@ -536,8 +537,12 @@ 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.
> +	 * Any channel below ctrl_count is guaranteed to have a valid
> +	 * entry in usb_if->dev[].
> +	 */
> +	if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
> +		return -EINVAL;
>  
>  	dev = usb_if->dev[pucan_msg_get_channel(rm)];
>  	netdev = dev->netdev;

[Severity: High]
Does this actually guarantee that the usb_if->dev[] entry is valid?

Since sibling channels are initialized sequentially during device probe, can
this lead to a NULL pointer dereference? For example, if can0 is brought up
and receives a message from the shared USB endpoint with a channel number
for can1 before can1 completes its initialization, usb_if->dev[channel]
might still be NULL, causing a crash when accessing dev->netdev.

Additionally, what happens during device disconnect? If peak_usb_disconnect()
frees sibling channels in reverse order (e.g., can1 then can0), the freed
channel's pointer seems to remain in the shared usb_if->dev[] array. If can0
is still active and receives a message for the freed channel, could this
result in a use-after-free?

This same pattern appears to be repeated in pcan_usb_fd_decode_status(),
pcan_usb_fd_decode_error(), and pcan_usb_fd_decode_overrun() modified
further down in this patch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-canfd_check_channel_idx-v1-0-0a3bb82f4e09@peak-system.fr?part=4

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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 check `channel >= usb_if->adapter->ctrl_count` only
  keeps the index inside dev[].
- [Low] The commit message for "can: peak_usb: validate channel numbers
  in PCAN-USB FD" says the patch "prevents potential out-of-bounds…

--

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

This patch changes the channel number check in the four PCAN-USB FD
decoders. The old bound was ARRAY_SIZE(usb_if->dev). The new bound is the
adapter's ctrl_count, read through an adapter pointer that is now stored in
struct pcan_usb_fd_if. Invalid channels now return -EINVAL instead of
-ENOMEM.

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

[ ... ]

> 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" the right description here? This was the old check:

	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))

dev[] has PCAN_USB_MAX_CHANNEL (2) entries. No index outside dev[0..1]
could get past that check before this patch.

What this patch seems to fix is a NULL pointer dereference on the
single-channel adapters, PCAN-USB FD and PCAN-Chip USB. Both use
.ctrl_count = PCAN_USBFD_CHANNEL_COUNT (1). pcan_usb_fd_init() only fills
usb_if->dev[dev->ctrl_idx], and usb_if comes from kzalloc. On those
adapters dev[1] therefore stays NULL.

Before this patch, a device record with channel 1 passed the "< 2" check.
Then, in URB completion context:

  pcan_usb_fd_decode_canmsg(), _decode_status() and _decode_overrun()
      read dev->netdev
  pcan_usb_fd_decode_error()
      wrote pdev->bec through container_of(NULL)

Could the commit message name this device-triggered NULL dereference and
the affected adapters? There is no Cc: stable tag, so this matters for
backport decisions.

The "entire message buffer is discarded" wording also reads as if this
patch adds that behaviour. The goto fail path in pcan_usb_fd_decode_buf()
is unchanged. Records decoded before the bad one have already been passed
up via netif_rx(), and only the bad record and the ones after it are
dropped.

On the -EINVAL change: pcan_usb_fd_decode_error() and
pcan_usb_fd_decode_overrun() already returned -EINVAL before this patch.
peak_usb_read_bulk_callback() also ignores the returned error code.

> 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..ddbb2be1de0dd 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c

[ ... ]

> @@ -536,8 +537,12 @@ 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.
> +	 * Any channel below ctrl_count is guaranteed to have a valid
> +	 * entry in usb_if->dev[].
> +	 */
> +	if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
> +		return -EINVAL;
>  
>  	dev = usb_if->dev[pucan_msg_get_channel(rm)];
>  	netdev = dev->netdev;

[Severity: High]
Does this guarantee hold? The ctrl_count check keeps the index inside
dev[], but nothing makes sure usb_if->dev[channel] points to a live
device.

On the 2-channel adapters (PCAN-USB Pro FD and PCAN-USB X6, ctrl_count =
PCAN_USBPROFD_CHANNEL_COUNT = 2), the new check is the same as the old
ARRAY_SIZE() check.

pcan_usb_fd_decode_status(), pcan_usb_fd_decode_error() and
pcan_usb_fd_decode_overrun() repeat the same comment and check.

The first case is a NULL entry during probe. peak_usb_probe() creates
channels one at a time, and peak_usb_create_dev() registers the netdev
before it calls dev_init():

peak_usb_create_dev()
    register_candev(netdev);
    ...
    dev->adapter->dev_init(dev);
        pcan_usb_fd_init()
            pdev->usb_if->dev[dev->ctrl_idx] = dev;

Once channel 0 is registered and initialised, can0 can be brought up while
dev[1] is still NULL. The window covers channel 0's dev_set_bus() and
dev_get_can_channel_id() bulk commands, which the device can stall for up
to 1000 ms each. It also covers register_candev() for can1.

Opening can0 submits RX URBs on the bulk-IN endpoint that both channels
share. pcan_usb_fd_decode_buf() routes each record by its own channel
field.

Could a channel-1 record then pass the "1 >= 2" test and dereference NULL
through dev->netdev here? The same would apply to dev->can.state in
pcan_usb_fd_decode_status() and to the write to pdev->bec in
pcan_usb_fd_decode_error().

The second case is a dangling entry when probe fails. If channel 1's
dev_set_bus() fails after dev_init(), peak_usb_create_dev() takes this
path:

adap_dev_free:
	if (dev->adapter->dev_free)
		dev->adapter->dev_free(dev);

lbl_unregister_candev:
	unregister_candev(netdev);

pcan_usb_fd_free() does nothing for a device that has prev_siblings set,
and it never clears usb_if->dev[1]:

static void pcan_usb_fd_free(struct peak_usb_device *dev)
{
	/* last device: can free shared objects now */
	if (!dev->prev_siblings && !dev->next_siblings) {

dev1 is then freed. peak_usb_probe() calls peak_usb_disconnect() directly,
without usb_disable_interface(), so can0's RX URBs are still live.

Can a channel-1 record arriving in that window reach freed memory? That
includes the write in pcan_usb_fd_decode_error().

Disconnects started by the USB core (unplug, sysfs unbind, rmmod) do not
seem to be affected. peak_usb_driver does not set soft_unbind, so
usb_unbind_interface() kills the URBs before ->disconnect().

The follow-up commit "can: peak_usb: harden PCAN-USB message validation"
only touches pcan_usb.c and does not cover this.

Would it be more complete to treat a NULL usb_if->dev[ch] as a malformed
message in each FD decoder? That would also cover the single-channel case
this patch targets.

Clearing the entry on per-channel teardown or failure would handle the
dangling case. The comment would also need to be corrected.

[ ... ]

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

[PATCH 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 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 (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 fix is incomplete: the PCAN_USB_REC_BUSEVT status record
  still allows an out-of-bounds read.

Pre-existing issues:
- [Medium] The encoder and decoder disagree about the SRR 'client private
  id' byte on RTR frames.

--

can: peak_usb: harden PCAN-USB message validation

This commit adds bounds checks to the PCAN-USB RX parser. In
pcan_usb_decode_data() the SRR client private id byte is now checked
against mc->end before it is skipped. In pcan_usb_decode_msg() the
status/len byte is now checked against mc->end before it is read.

> 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 do the encoder and decoder agree on the
SRR private id byte for RTR frames?

When CAN_CTRLMODE_LOOPBACK is set, pcan_usb_encode_msg() sets
PCAN_USB_TX_SRR. It then appends the writer id outside the non-RTR data
block:

	/* 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;

pcan_usb_decode_data() only skips that byte, and now checks its bounds,
in the non-RTR else branch.

Suppose the firmware echoes a self-received RTR frame with the private id
byte, as the TX layout suggests. Could the RX cursor then fall one byte
behind? In that case the private id byte would be parsed as the next
record's status/len byte, and the rest of the URB would be misparsed or
rejected with -EINVAL.

The code doesn't document whether the firmware includes that byte for
RTR echoes. This dates from the earlier SRR handling (3a7939495ce8) and
is not an out-of-bounds access.

>  
>  		/* 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);

[Severity: High]
The commit message says the patch rejects malformed buffers "whose
contents would otherwise lead to accesses outside the received USB data
area". Can a PCAN_USB_REC_BUSEVT status record still read past mc->end?

Before dispatch, pcan_usb_decode_status() only checks that the f and n
bytes are in bounds:

	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;

After this, mc->ptr can equal mc->end. Now take a record where:

  - PCAN_USB_STATUSLEN_TIMESTAMP is clear
  - f is PCAN_USB_REC_BUSEVT
  - n is PCAN_USB_ERR_CNT_DEC or PCAN_USB_ERR_CNT_INC

pcan_usb_handle_bus_evt() then reads two bytes with no check against
mc->end:

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

pcan_usb_decode_status() does have a later check:

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

But that check only runs after these reads. It also uses the DLC nibble
supplied by the device, which can be 0.

peak_usb_start() allocates the RX buffer as
kmalloc(dev->adapter->rx_buffer_size), and PCAN_USB_RX_BUFFER_SIZE is 64.
Say a 64-byte transfer ends with the 3-byte BUSEVT header
{0x40, 0x05, 0x00} at offsets 61 to 63. Wouldn't mc->ptr[1] and
mc->ptr[2] then be buf[65] and buf[66], past the slab object? For shorter
transfers, the same reads would return stale bytes from earlier URBs.

These values are stored in pdev->bec. pcan_usb_get_berr_counter() returns
them to userspace, and pcan_usb_decode_error() copies them into error
frames:

			cf->data[6] = mc->pdev->bec.txerr;
			cf->data[7] = mc->pdev->bec.rxerr;

Does the fix also need a check like this in pcan_usb_handle_bus_evt(),
before the reads?

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

[ ... ]

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