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

[Stéphane Grosjean]

From: Stéphane Grosjean <s.grosjean@xxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxx>
---
 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