[PATCH v2] USB: serial: sierra: validate URB length in instat callback

[PATCH v2] USB: serial: sierra: validate URB length in instat callback

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

sierra_instat_callback() reads request fields and one byte past the
usb_ctrlrequest header without first checking that urb->actual_length
is large enough.  A short USB interrupt transfer could cause an
out-of-bounds read.

Add two length checks matching the pattern already present in the
option driver (option_instat_callback): one for the request header
before accessing bRequestType, and one for the line-state data byte
inside the serial-state notification handler.  Use dev_err() since
failing these checks causes the interrupt URB not to be resubmitted.

Fixes: 033a3fb980b041 ("USB: Sierra Wireless driver update")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v2 (Johan Hovold):
 - Split into two checks: one for the header before accessing
   bRequestType, and one for the line-state data byte inside
   the serial-state handler, matching the option driver pattern
 - Use dev_err() instead of dev_dbg() since early return prevents
   further interrupts

 drivers/usb/serial/sierra.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index 6e443aacae07d..03ed76dfbb2e3 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -575,10 +575,23 @@ static void sierra_instat_callback(struct urb *urb)
 				__func__);
 			return;
 		}
+		if (urb->actual_length < sizeof(*req_pkt)) {
+			dev_err(&port->dev, "%s: short transfer\n",
+				__func__);
+			return;
+		}
 		if ((req_pkt->bRequestType == 0xA1) &&
 				(req_pkt->bRequest == 0x20)) {
 			int old_dcd_state;
-			unsigned char signals = *((unsigned char *)
+			unsigned char signals;
+
+			if (urb->actual_length < sizeof(*req_pkt) + 1) {
+				dev_err(&port->dev,
+					"%s: short interrupt transfer\n",
+					__func__);
+				return;
+			}
+			signals = *((unsigned char *)
 					urb->transfer_buffer +
 					sizeof(struct usb_ctrlrequest));
 
-- 
2.47.3

Re: [PATCH v2] USB: serial: sierra: validate URB length in instat callback

[sashiko-bot]

> From: Tristan Madani <tristan@talencesecurity.com>
> 
> sierra_instat_callback() reads request fields and one byte past the
> usb_ctrlrequest header without first checking that urb->actual_length
> is large enough.  A short USB interrupt transfer could cause an
> out-of-bounds read.
> 
> Add two length checks matching the pattern already present in the
> option driver (option_instat_callback): one for the request header
> before accessing bRequestType, and one for the line-state data byte
> inside the serial-state notification handler.  Use dev_err() since
> failing these checks causes the interrupt URB not to be resubmitted.
> 
> Fixes: 033a3fb980b041 ("USB: Sierra Wireless driver update")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019103849.3716324-1-tristmd@gmail.com?part=1