usb: usbip: fix integer overflow in usbip_recv_iso()

usbip_recv_iso() computes the iso descriptor buffer size as:

    int size = np * sizeof(*iso);

where np comes straight from the wire (urb->number_of_packets, set by usbip_pack_ret_submit() before we get here). With np = 0x10000001 and sizeof(*iso) == 16 the product is 0x100000010 which truncates to 16 on a 32-bit int. kzalloc(16) succeeds but the following receive loop writes np * 16 bytes into it - game over.

USBIP_MAX_ISO_PACKETS (1024) already exists in usbip_common.h for the submit path but was never enforced on the receive side.

Clamp np to [1, USBIP_MAX_ISO_PACKETS] and switch to kcalloc() so the allocator itself can catch overflows in the future. Fold the existing np == 0 early return into the new bounds check.

usbip_pack_ret_submit() already copied the bogus np into urb->number_of_packets before we run, so just returning -EPROTO is not enough - processcompl() in the HCD will still iterate that many iso_frame_desc entries when it completes the failed URB. Zero out urb->number_of_packets before bailing to prevent that secondary crash (confirmed on 6.12.0, processcompl+0x63 with CR2 in unmapped slab).

---
Changes in v2:
- Fold np == 0 early return into the new bounds check (Greg)
- Switch from kmalloc_array() to kcalloc() (Greg)
- Keep size variable to avoid repeating np * sizeof(*iso) (Greg)
- Use proper block comment style (Greg)
- Drop Reported-by (author is signer)
- Drop [email protected] CC

 drivers/usb/usbip/usbip_common.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

index a2b2da1..8b6eb74 100644
@@ -662,7 +662,7 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	void *buff;
 	struct usbip_iso_packet_descriptor *iso;
 	int np = urb->number_of_packets;
-	int size = np * sizeof(*iso);
+	int size;
 	int i;
 	int ret;
 	int total_length = 0;

@@ -670,11 +670,21 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	if (!usb_pipeisoc(urb->pipe))
 		return 0;
 
-	/* my Bluetooth dongle gets ISO URBs which are np = 0 */
-	if (np == 0)
-		return 0;
+	if (np <= 0 || np > USBIP_MAX_ISO_PACKETS) {
+		dev_err(&urb->dev->dev,
+			"recv iso: invalid number_of_packets %d\n", np);
+		/*
+		 * usbip_pack_ret_submit() already set urb->number_of_packets
+		 * from the wire.  Zero it so processcompl() does not iterate
+		 * OOB descriptors on the way out.
+		 */
+		urb->number_of_packets = 0;
+		return -EPROTO;
+	}
+
+	size = np * sizeof(*iso);
 
-	buff = kzalloc(size, GFP_KERNEL);
+	buff = kcalloc(np, sizeof(*iso), GFP_KERNEL);
 	if (!buff)
 		return -ENOMEM;
 

2.34.1

usbip_recv_iso() sums each frame's actual_length into an int accumulator without checking the individual values first:

    total_length += urb->iso_frame_desc[i].actual_length;

A malicious server can send actual_length = 0xFFFFFFFC for one frame and a small value for the other, making the signed sum wrap around to match urb->actual_length. The sanity check passes, and usbip_pad_iso() later computes a negative actualoffset, feeding it to memmove() as a source pointer - reads before the allocation, leaked to userspace via USBDEVFS_REAPURB.

Reject any frame whose actual_length exceeds transfer_buffer_length (one frame can't carry more data than the whole buffer), and widen the accumulator to u32 so that many moderately-large frames can't wrap it either.

---
Changes in v2:
- Drop Reported-by (author is signer)

 drivers/usb/usbip/usbip_common.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

index 8b6eb74..fd620e9 100644
@@ -665,7 +665,7 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	int size;
 	int i;
 	int ret;
-	int total_length = 0;
+	u32 total_length = 0;
 
 	if (!usb_pipeisoc(urb->pipe))
 		return 0;

@@ -706,14 +706,23 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	for (i = 0; i < np; i++) {
 		usbip_iso_packet_correct_endian(&iso[i], 0);
 		usbip_pack_iso(&iso[i], &urb->iso_frame_desc[i], 0);
+		if (urb->iso_frame_desc[i].actual_length >
+				(unsigned int)urb->transfer_buffer_length) {
+			dev_err(&urb->dev->dev,
+				"recv iso: frame actual_length %u exceeds buffer %d\n",
+				urb->iso_frame_desc[i].actual_length,
+				urb->transfer_buffer_length);
+			kfree(buff);
+			return -EPROTO;
+		}
 		total_length += urb->iso_frame_desc[i].actual_length;
 	}
 
 	kfree(buff);
 
-	if (total_length != urb->actual_length) {
+	if (total_length != (u32)urb->actual_length) {
 		dev_err(&urb->dev->dev,
-			"total length of iso packets %d not equal to actual length of buffer %d\n",
+			"total length of iso packets %u not equal to actual length of buffer %d\n",
 			total_length, urb->actual_length);
 
 		if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC)

2.34.1

usbip_pad_iso() repositions ISO frame data within the transfer buffer via memmove(). Neither the source offset (actualoffset, derived by subtracting wire-supplied actual_length values) nor the destination offset (iso_frame_desc[i].offset, taken directly from the wire) is bounds-checked.

If a crafted actual_length wraps actualoffset negative through the subtraction (see patch 2/3 for the root cause), the memmove source points before the allocation - slab OOB read, data returned to userspace.

Independently, iso_frame_desc[i].offset is never validated against transfer_buffer_length. Setting offset past the end of the buffer gives a fully controlled OOB write into whatever sits next in the slab - confirmed with offset=400 on a 392-byte buffer, 64-byte write.

Add bounds checks for both the source and destination ranges before each memmove call. Use unsigned comparisons after the sign check on actualoffset to avoid signed/unsigned conversion surprises.

---
Changes in v2:
- Fix indentation to pass checkpatch (Greg)
- Reword comments to describe what is being validated (Greg)
- Drop Reported-by (author is signer)

 drivers/usb/usbip/usbip_common.c | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

index fd620e9..8ebaaea 100644
@@ -770,6 +770,42 @@ void usbip_pad_iso(struct usbip_device *ud, struct urb *urb)
 	 */
 	for (i = np-1; i > 0; i--) {
 		actualoffset -= urb->iso_frame_desc[i].actual_length;
+
+		/*
+		 * Validate source range: actualoffset can go negative
+		 * via crafted actual_length values from the wire.
+		 */
+		if (actualoffset < 0 ||
+		    (unsigned int)actualoffset >
+				(unsigned int)urb->transfer_buffer_length ||
+		    urb->iso_frame_desc[i].actual_length >
+				(unsigned int)urb->transfer_buffer_length -
+				(unsigned int)actualoffset) {
+			dev_err(&urb->dev->dev,
+				"pad_iso: bad src off=%d len=%u bufsz=%d\n",
+				actualoffset,
+				urb->iso_frame_desc[i].actual_length,
+				urb->transfer_buffer_length);
+			return;
+		}
+
+		/*
+		 * Validate destination range: iso_frame_desc[i].offset
+		 * is wire-supplied and must not exceed the buffer.
+		 */
+		if (urb->iso_frame_desc[i].offset >
+				(unsigned int)urb->transfer_buffer_length ||
+		    urb->iso_frame_desc[i].actual_length >
+				(unsigned int)urb->transfer_buffer_length -
+				urb->iso_frame_desc[i].offset) {
+			dev_err(&urb->dev->dev,
+				"pad_iso: bad dst off=%u len=%u bufsz=%d\n",
+				urb->iso_frame_desc[i].offset,
+				urb->iso_frame_desc[i].actual_length,
+				urb->transfer_buffer_length);
+			return;
+		}
+
 		memmove(urb->transfer_buffer + urb->iso_frame_desc[i].offset,
 			urb->transfer_buffer + actualoffset,
 			urb->iso_frame_desc[i].actual_length);

2.34.1

Hi Kelvin,

Your series hardens usbip_recv_iso() and usbip_pad_iso() against malicious number_of_packets values, but the bad value still lands in urb->number_of_packets via usbip_pack_ret_submit() before those checks run.

The patch below validates at the source — in usbip_pack_ret_submit() before the overwrite — and checks against the original urb->number_of_packets (the actual allocation bound) rather than USBIP_MAX_ISO_PACKETS. This is a tighter check because the URB may have been allocated for far fewer than 1024 packets.

This could complement your series as an additional layer, or stand alone. Would be glad to rework this however the maintainers see fit — whether folded into your series or submitted separately.

---

From: Nathan Rebello <[email protected]>
Subject: [PATCH] usbip: vhci: reject RET_SUBMIT with inflated
number_of_packets

When a USB/IP client receives a RET_SUBMIT response, usbip_pack_ret_submit() unconditionally overwrites urb->number_of_packets from the network PDU. This value is subsequently used as the loop bound in usbip_recv_iso() and usbip_pad_iso() to iterate over urb->iso_frame_desc[], a flexible array whose size was fixed at URB allocation time based on the *original* number_of_packets from the CMD_SUBMIT.

A malicious USB/IP server can set number_of_packets in the response to a value larger than what was originally submitted, causing a heap out-of-bounds write when usbip_recv_iso() writes to urb->iso_frame_desc[i] beyond the allocated region.

KASAN confirmed this with kernel 7.0.0-rc5:

  BUG: KASAN: slab-out-of-bounds in usbip_recv_iso+0x46a/0x640
  Write of size 4 at addr ffff888106351d40 by task vhci_rx/69

The buggy address is located 0 bytes to the right of
allocated 320-byte region [ffff888106351c00, ffff888106351d40)

The server side (stub_rx.c) and gadget side (vudc_rx.c) already validate number_of_packets in the CMD_SUBMIT path since commits c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input") and b78d830f0049 ("usbip: fix vudc_rx: harden CMD_SUBMIT path to handle malicious input"). The server side validates against USBIP_MAX_ISO_PACKETS because no URB exists yet at that point. On the client side we have the original URB, so we can use the tighter
bound: the response must not exceed the original number_of_packets.

This mirrors the existing validation of actual_length against transfer_buffer_length in usbip_recv_xbuff(), which checks the response value against the original allocation size.

Fix this by checking rpdu->number_of_packets against urb->number_of_packets in usbip_pack_ret_submit() before the overwrite. On violation, clamp to zero so that usbip_recv_iso() and usbip_pad_iso() safely return early.

---
 drivers/usb/usbip/usbip_common.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

@@ -470,7 +470,18 @@ static void usbip_pack_ret_submit(struct usbip_header *pdu, struct urb *urb,
 		urb->status		= rpdu->status;
 		urb->actual_length	= rpdu->actual_length;
 		urb->start_frame	= rpdu->start_frame;
-		urb->number_of_packets = rpdu->number_of_packets;
+		/*
+		 * The number_of_packets field determines the length of
+		 * iso_frame_desc[], which is a flexible array allocated
+		 * at URB creation time. A response must never claim more
+		 * packets than originally submitted; doing so would cause
+		 * an out-of-bounds write in usbip_recv_iso() and
+		 * usbip_pad_iso(). Clamp to zero on violation so both
+		 * functions safely return early.
+		 */
+		if (rpdu->number_of_packets < 0 ||
+		    rpdu->number_of_packets > urb->number_of_packets)
+			rpdu->number_of_packets = 0;
+		urb->number_of_packets = rpdu->number_of_packets;
 		urb->error_count	= rpdu->error_count;
 	}

}

Please submit it separately, on top of that series, to make it easier to review and apply.

thanks,

greg k-h

This id is not in the kernel tree :(