Skip to contentSkip to conversation

USB: serial: cypress_m8: validate interrupt packet headers

PATCH In review Zhang Cen 2 participants 2 messages May 22 – May 23

Subsystems (2)
USB USB serial
lore
Share & tools
lore

Zhang Cen

author
#0 lore raw

PATCH v2 1/1 USB: serial: cypress_m8: validate interrupt packet headers

cypress_read_int_callback() parses the interrupt-in buffer according to the selected Cypress packet format. Format 1 has a two-byte status/count header and format 2 has a one-byte combined status/count header. The usb-serial core sizes the interrupt-in buffer from the endpoint descriptor's wMaxPacketSize, and successful interrupt transfers can complete short when URB_SHORT_NOT_OK is not set.

Check that the completed packet contains the selected header before reading it. Malformed short reports are ignored and the interrupt URB is resubmitted through the existing retry path, preventing out-of-bounds header-byte reads.

KASAN report as below:
KASAN slab-out-of-bounds in cypress_read_int_callback+0x240/0x7f0

Read of size 1
Call trace:
  cypress_read_int_callback() (drivers/usb/serial/cypress_m8.c:1009)
  __usb_hcd_giveback_urb()
  dummy_timer()
Fixes 3416eaa1f8f8 ("USB: cypress_m8: Packet format is separate from characteristic size")
Assisted-by Codex:gpt-5.5
Signed-off-by Zhang Cen <[email protected]>

---
v2:
Check only urb->actual_length before reading the packet-format header. Reuse the existing i header-length variable instead of adding a new one. Shorten the KASAN trace in the commit message.

 drivers/usb/serial/cypress_m8.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/usb/serial/cypress_m8.c +12 -3
index afff1a0f4298b..49c0f3e379bd0 100644
@@ -1060,18 +1060,27 @@ static void cypress_read_int_callback(struct urb *urb)
 	default:
 	case packet_format_1:
 		/* This is for the CY7C64013... */
-		priv->current_status = data[0] & 0xF8;
-		bytes = data[1] + 2;
 		i = 2;
+		if (result < i)
+			break;
+		priv->current_status = data[0] & 0xF8;
+		bytes = data[1] + i;
 		break;
 	case packet_format_2:
 		/* This is for the CY7C63743... */
+		i = 1;
+		if (result < i)
+			break;
 		priv->current_status = data[0] & 0xF8;
 		bytes = (data[0] & 0x07) + 1;
-		i = 1;
 		break;
 	}
 	spin_unlock_irqrestore(&priv->lock, flags);
+	if (result < i) {
+		dev_dbg(dev, "%s - short packet received: %d bytes\n",
+			__func__, result);
+		goto continue_read;
+	}
 	if (result < bytes) {
 		dev_dbg(dev,
 			"%s - wrong packet size - received %d bytes but packet said %d bytes\n",
Signature · 1 line
2.43.0
maintainer
#1 lore raw
Quoted 20 lines · Zhang Cen

cypress_read_int_callback() parses the interrupt-in buffer according to the selected Cypress packet format. Format 1 has a two-byte status/count header and format 2 has a one-byte combined status/count header. The usb-serial core sizes the interrupt-in buffer from the endpoint descriptor's wMaxPacketSize, and successful interrupt transfers can complete short when URB_SHORT_NOT_OK is not set.

Check that the completed packet contains the selected header before reading it. Malformed short reports are ignored and the interrupt URB is resubmitted through the existing retry path, preventing out-of-bounds header-byte reads.

KASAN report as below:
KASAN slab-out-of-bounds in cypress_read_int_callback+0x240/0x7f0

Read of size 1
Call trace:
  cypress_read_int_callback() (drivers/usb/serial/cypress_m8.c:1009)
  __usb_hcd_giveback_urb()
  dummy_timer()
Quoted 8 lines
Fixes 3416eaa1f8f8 ("USB: cypress_m8: Packet format is separate from characteristic size")
Assisted-by Codex:gpt-5.5
Signed-off-by Zhang Cen <[email protected]>

---
v2:
Check only urb->actual_length before reading the packet-format header. Reuse the existing i header-length variable instead of adding a new one. Shorten the KASAN trace in the commit message.

Thanks for the v2.

Quoted 9 lines
 drivers/usb/serial/cypress_m8.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/usb/serial/cypress_m8.c +0 -0
index afff1a0f4298b..49c0f3e379bd0 100644
@@ -1060,18 +1060,27 @@ static void cypress_read_int_callback(struct urb *urb)
 	default:

drivers/usb/serial/cypress_m8.c static void cypress_read_int_callback(struct urb *urb) · line 1068

Quoted 10 lines
drivers/usb/serial/cypress_m8.c +4 -2
 	case packet_format_1:
 		/* This is for the CY7C64013... */
-		priv->current_status = data[0] & 0xF8;
-		bytes = data[1] + 2;
 		i = 2;
+		if (result < i)
+			break;
+		priv->current_status = data[0] & 0xF8;
+		bytes = data[1] + i;
 		break;

I know I asked you to move the initialisation of i (the header length), but when applying I ended up changing this so that the sanity checks use constants instead, which I found more readable.

The end result is here:

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-linus&id=9f9bfc80c67f35a275820da7e83a35dface08281

Johan