[PATCH v2 1/3] usb: usbip: fix integer overflow in usbip_recv_iso()

[PATCH v2 1/3] usb: usbip: fix integer overflow in usbip_recv_iso()

[Kelvin Mbogo]

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).

Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>
---
 drivers/usb/usbip/usbip_common.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index a2b2da1..8b6eb74 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -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

[PATCH v2 2/3] usb: usbip: validate iso frame actual_length in usbip_recv_iso()

[Kelvin Mbogo]

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.

Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>
---
 drivers/usb/usbip/usbip_common.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 8b6eb74..fd620e9 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -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

[PATCH v2 3/3] usb: usbip: fix OOB read/write in usbip_pad_iso()

[Kelvin Mbogo]

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.

Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>
---
 drivers/usb/usbip/usbip_common.c | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index fd620e9..8ebaaea 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -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

Re: [PATCH v2 1/3] usb: usbip: fix integer overflow in usbip_recv_iso()

[Greg KH]

On Wed, Mar 25, 2026 at 01:36:38PM +0300, Kelvin Mbogo wrote:
> 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).
> 
> Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>
> ---
>  drivers/usb/usbip/usbip_common.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> index a2b2da1..8b6eb74 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -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
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH v2 2/3] usb: usbip: validate iso frame actual_length in usbip_recv_iso()

[Kelvin Mbogo]

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.

Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>
---
Changes in v2:
  - Drop Reported-by (author is signer)

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

diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 8b6eb74..fd620e9 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -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