[PATCH 1/2] can: esd_usb: validate received message length before use

esd_usb_read_bulk_callback() walks a sequence of variable-length
messages out of the RX URB buffer. The only length check,
"pos > urb->actual_length", runs *after* the current message has been
dispatched and after @pos has been advanced, so it can neither protect
the message being processed nor stop the loop:

 - The per-message handlers dereference fixed offsets of the message.
   esd_usb_rx_can_msg() reads the 32-bit CAN id at offset 8 and copies
   up to CANFD_MAX_DLEN payload bytes from offset 12; the error-event
   path reads four status bytes at offset 12; esd_usb_tx_done_msg()
   reads the tx handle at offset 4. A malicious or malfunctioning
   device can place a short ESD_USB_CMD_CAN_RX header near the end of
   actual_length so that these reads fall past the buffer, leaking
   adjacent kernel heap into a received CAN(-FD) skb.

 - hdr.len is the message length in 32-bit words. A message with
   hdr.len == 0 never advances @pos, spinning this URB-completion
   softirq forever.

Validate the header and the declared message length before dispatch:
reject a message whose header is not fully present, whose length is
zero, or which extends past the received data, and advance @pos by the
validated length. Pass the validated length to the message handlers so
they can confirm that the fields they read (and the payload they copy)
were actually received.

This is the esd_usb counterpart of the kvaser_usb_leaf fix,
commit 0293dd153f9d ("can: kvaser_usb_leaf: kvaser_usb_leaf_wait_cmd():
validate received command extents"). esd_usb has the same unbounded
receive-buffer walk and was not touched by that fix.

Reproduced with USB_RAW_GADGET + dummy_hcd on a KASAN build: a bulk-IN
frame whose first message has hdr.len == 255 (advancing @pos to 1020)
followed by an ESD_USB_CMD_CAN_RX header at offset 1020 yields

  BUG: KASAN: slab-out-of-bounds in esd_usb_read_bulk_callback+0x38f/0xd70
  Read of size 4 at addr ffff88800c0a9c04 by task init/1
   kasan_check_range
   esd_usb_read_bulk_callback+0x38f/0xd70
   __usb_hcd_giveback_urb
   dummy_timer
  The buggy address belongs to the object at ffff88800c0a9800
   which belongs to the cache kmalloc-1k of size 1024
  The buggy address is located 4 bytes to the right of
   allocated 1024-byte region [ffff88800c0a9800, ffff88800c0a9c00)

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Cc: [email protected]
Signed-off-by: Yiran Qiu <[email protected]>
---
 drivers/net/can/usb/esd_usb.c | 69 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index f41d4a0d140f7..13356e68f3f60 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -298,7 +298,7 @@ struct esd_usb_net_priv {
 };
 
 static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
-			     union esd_usb_msg *msg)
+			     union esd_usb_msg *msg, unsigned int msg_len)
 {
 	struct net_device_stats *stats = &priv->netdev->stats;
 	struct can_frame *cf;
@@ -306,8 +306,14 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
 	u32 id = le32_to_cpu(msg->rx.id) & ESD_USB_IDMASK;
 
 	if (id == ESD_USB_EV_CAN_ERROR_EXT) {
-		u8 state = msg->rx.ev_can_err_ext.status;
-		u8 ecc = msg->rx.ev_can_err_ext.ecc;
+		u8 state;
+		u8 ecc;
+
+		if (msg_len < offsetofend(struct esd_usb_rx_msg, ev_can_err_ext))
+			return;
+
+		state = msg->rx.ev_can_err_ext.status;
+		ecc = msg->rx.ev_can_err_ext.ecc;
 
 		priv->bec.rxerr = msg->rx.ev_can_err_ext.rec;
 		priv->bec.txerr = msg->rx.ev_can_err_ext.tec;
@@ -395,7 +401,7 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
 }
 
 static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
-			       union esd_usb_msg *msg)
+			       union esd_usb_msg *msg, unsigned int msg_len)
 {
 	struct net_device_stats *stats = &priv->netdev->stats;
 	struct can_frame *cf;
@@ -407,10 +413,19 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
 	if (!netif_device_present(priv->netdev))
 		return;
 
+	/* The device controls the message length; make sure the fixed rx
+	 * header (up to and including the CAN id) was actually received
+	 * before it is dereferenced.
+	 */
+	if (msg_len < offsetofend(struct esd_usb_rx_msg, id)) {
+		stats->rx_length_errors++;
+		return;
+	}
+
 	id = le32_to_cpu(msg->rx.id);
 
 	if (id & ESD_USB_EVENT) {
-		esd_usb_rx_event(priv, msg);
+		esd_usb_rx_event(priv, msg, msg_len);
 	} else {
 		if (msg->rx.dlc & ESD_USB_FD) {
 			skb = alloc_canfd_skb(priv->netdev, &cfd);
@@ -446,6 +461,15 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
 		if (id & ESD_USB_EXTID)
 			cfd->can_id |= CAN_EFF_FLAG;
 
+		/* Reject a frame that claims more payload than was actually
+		 * received, to avoid copying past the URB buffer.
+		 */
+		if (len > msg_len - offsetofend(struct esd_usb_rx_msg, id)) {
+			stats->rx_length_errors++;
+			dev_kfree_skb_any(skb);
+			return;
+		}
+
 		memcpy(cfd->data, msg->rx.data_fd, len);
 		stats->rx_bytes += len;
 		stats->rx_packets++;
@@ -455,7 +479,7 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
 }
 
 static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
-				union esd_usb_msg *msg)
+				union esd_usb_msg *msg, unsigned int msg_len)
 {
 	struct net_device_stats *stats = &priv->netdev->stats;
 	struct net_device *netdev = priv->netdev;
@@ -464,6 +488,9 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
 	if (!netif_device_present(netdev))
 		return;
 
+	if (msg_len < offsetofend(struct esd_usb_tx_done_msg, hnd))
+		return;
+
 	context = &priv->tx_contexts[msg->txdone.hnd & (ESD_USB_MAX_TX_URBS - 1)];
 
 	if (!msg->txdone.status) {
@@ -507,8 +534,27 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
 
 	while (pos < urb->actual_length) {
 		union esd_usb_msg *msg;
+		unsigned int msg_len;
+
+		/* The header must be fully present before hdr.len / hdr.cmd
+		 * (and the net index below) are read.
+		 */
+		if (pos + sizeof(struct esd_usb_header_msg) > urb->actual_length) {
+			dev_err(dev->udev->dev.parent, "format error\n");
+			break;
+		}
 
 		msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);
+		msg_len = msg->hdr.len * sizeof(u32); /* convert to # of bytes */
+
+		/* A zero-length message would never advance @pos and would
+		 * spin this URB-completion softirq forever; a message must
+		 * also fit within the received data.
+		 */
+		if (msg->hdr.len == 0 || msg_len > urb->actual_length - pos) {
+			dev_err(dev->udev->dev.parent, "format error\n");
+			break;
+		}
 
 		switch (msg->hdr.cmd) {
 		case ESD_USB_CMD_CAN_RX:
@@ -517,7 +563,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
 				break;
 			}
 
-			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);
+			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg, msg_len);
 			break;
 
 		case ESD_USB_CMD_CAN_TX:
@@ -527,16 +573,11 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
 			}
 
 			esd_usb_tx_done_msg(dev->nets[msg->txdone.net],
-					    msg);
+					    msg, msg_len);
 			break;
 		}
 
-		pos += msg->hdr.len * sizeof(u32); /* convert to # of bytes */
-
-		if (pos > urb->actual_length) {
-			dev_err(dev->udev->dev.parent, "format error\n");
-			break;
-		}
+		pos += msg_len;
 	}
 
 resubmit_urb:

-- 
2.55.0