[PATCH] usbip: fix number_of_packets corruption for non-isochronous transfers

[PATCH] usbip: fix number_of_packets corruption for non-isochronous transfers

[Markus Mikonsaari]

In the USB/IP protocol, number_of_packets is set to 0xffffffff (-1)
by sender when the transfer is not isochronous.
usbip_pack_pdu() copies this wire value into urb->number_of_packets
unconditionally.

A host controller driver may compute the iso_frame_desc memory requirements
directly from number_of_packets without independently validating it
against the pipe type which produces an undersized allocation.
On dwc_otg, this manifests as a slab-out-of-bounds write in
dwc_otg_hcd_urb_alloc() during a USB/IP attach involving a non-isochronous
transfer.

Correct the number_of_packets to the value the urb was actually
allocated for immediately after usbip_pack_pdu() overwrites it.

Signed-off-by: Markus Mikonsaari <markus.mikonsaari@gofore.com>
---
 drivers/usb/usbip/stub_rx.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 1e9ae578810d..baf511024da2 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -567,6 +567,17 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 		}
 
 		usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
+		/*
+		 * number_of_packets is set to -1 by the sender when the transfer is
+		 * not isochronous.
+		 * usbip_pack_pdu() copies this wire value into urb->number_of_packets
+		 * unconditionally, instead of using the correct value in np which was
+		 * used to allocate the urb above. For a non-isochronous transfer this
+		 * leaves number_of_packets at -1 which downstream consumers of this urb
+		 * like host-controller drivers use to allocate iso_frame_desc storage.
+		 * Restore it to what the urb was actually allocated for.
+		 */
+		priv->urbs[0]->number_of_packets = np;
 	} else {
 		for_each_sg(sgl, sg, nents, i) {
 			priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
@@ -579,6 +590,8 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 			usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
 			priv->urbs[i]->transfer_buffer = sg_virt(sg);
 			priv->urbs[i]->transfer_buffer_length = sg->length;
+			/* see comment about number_of_packets above */
+			priv->urbs[i]->number_of_packets = 0;
 		}
 		priv->sgl = sgl;
 	}
-- 
2.55.0

Re: [PATCH] usbip: fix number_of_packets corruption for non-isochronous transfers

[gregkh]

On Wed, Sep 09, 2026 at 08:09:48AM +0000, Markus Mikonsaari wrote:
> In the USB/IP protocol, number_of_packets is set to 0xffffffff (-1)
> by sender when the transfer is not isochronous.
> usbip_pack_pdu() copies this wire value into urb->number_of_packets
> unconditionally.
> 
> A host controller driver may compute the iso_frame_desc memory requirements
> directly from number_of_packets without independently validating it
> against the pipe type which produces an undersized allocation.

What driver does that?

> On dwc_otg, this manifests as a slab-out-of-bounds write in
> dwc_otg_hcd_urb_alloc() during a USB/IP attach involving a non-isochronous
> transfer.
> 
> Correct the number_of_packets to the value the urb was actually
> allocated for immediately after usbip_pack_pdu() overwrites it.
> 
> Signed-off-by: Markus Mikonsaari <markus.mikonsaari@gofore.com>

How was this found and tested?

And did you forget an Assisted-by: tag?

> ---
>  drivers/usb/usbip/stub_rx.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index 1e9ae578810d..baf511024da2 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -567,6 +567,17 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  		}
>  
>  		usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
> +		/*
> +		 * number_of_packets is set to -1 by the sender when the transfer is
> +		 * not isochronous.
> +		 * usbip_pack_pdu() copies this wire value into urb->number_of_packets
> +		 * unconditionally, instead of using the correct value in np which was
> +		 * used to allocate the urb above. For a non-isochronous transfer this
> +		 * leaves number_of_packets at -1 which downstream consumers of this urb
> +		 * like host-controller drivers use to allocate iso_frame_desc storage.
> +		 * Restore it to what the urb was actually allocated for.
> +		 */
> +		priv->urbs[0]->number_of_packets = np;
>  	} else {
>  		for_each_sg(sgl, sg, nents, i) {
>  			priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
> @@ -579,6 +590,8 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  			usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
>  			priv->urbs[i]->transfer_buffer = sg_virt(sg);
>  			priv->urbs[i]->transfer_buffer_length = sg->length;
> +			/* see comment about number_of_packets above */

What comment?  That's not the best way to do this...

thanks,

greg k-h

Re: [PATCH] usbip: fix number_of_packets corruption for non-isochronous transfers

[Markus Mikonsaari]

Hi,

> What driver does that?
The DWC-OTG USB host controller is a part of the Raspberry Pi
fork of the Linux Kernel: https://github.com/nfeske/dwc_otg
Particularly dwc_otg_hcd_urb_alloc does:
 size = sizeof(*dwc_otg_urb) + 
   iso_desc_count * sizeof(struct dwc_otg_hcd_iso_packet_desc);
where iso_desc_count is urb->number_of_packets.

> How was this found and tested?
This issue was found and tested on an industrial box pc based on 
the rpi zero2w, the EDC-IPC1100. Every USB/IP attach resulted in a 
kernel panic on the device.

> And did you forget an Assisted-by: tag?
The fix itself is not generated by an LLM, but I did use it to aid me in
setting up the build and tests and commit message tone.
I will add the tag.

> What comment? That's not the best way to do this...
Right, sorry about that. I didn't want to pollute the file with explaining the same thing
twice. I will move the comment and the action into it's own fuction.

Thank you for taking the time to help me by reviewing and commenting!

- Markus

[PATCH v2] usbip: fix number_of_packets for non-iso transfers

[Markus Mikonsaari]

In the USB/IP protocol, number_of_packets is set to 0xffffffff (-1)
by sender when the transfer is not isochronous.
usbip_pack_pdu() copies this value into urb->number_of_packets.

A host controller driver may compute the iso_frame_desc size directly
from number_of_packets without independently validating it against 
the pipe type which produces an undersized allocation.
For example on a Raspberry Pi, the dwc_otg driver panics with a 
slab-out-of-bounds write in dwc_otg_hcd_urb_alloc() during a 
USB/IP attach involving a non-isochronous transfer.

Correct the number_of_packets to the value the urb was actually
allocated for immediately after usbip_pack_pdu() overwrites it.

Assisted-by: LLM
Signed-off-by: Markus Mikonsaari <markus.mikonsaari@gofore.com>
---
v2: 
  - Moved fix and comment into a static helper
  - Shortened summary
  - Added Assisted-by tag

 drivers/usb/usbip/stub_rx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 1e9ae578810d..d592fd8edd87 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -461,6 +461,20 @@ static int stub_recv_xbuff(struct usbip_device *ud, struct stub_priv *priv)
 	return ret;
 }
 
+/*
+ * number_of_packets is set to -1 by the USB/IP sender when the transfer
+ * is not isochronous.
+ * usbip_pack_pdu() copies this value into urb->number_of_packets.
+ * Leaving the number_of_packets at -1 can lead to
+ * downstream consumers of this urb (e.g. host-controller drivers)
+ * to allocate negative iso_frame_desc storage.
+ * Set it to what the urb was actually allocated for.
+ */
+static inline void stub_fixup_urb_number_of_packets(struct urb *urb, int np)
+{
+	urb->number_of_packets = np;
+}
+
 static void stub_recv_cmd_submit(struct stub_device *sdev,
 				 struct usbip_header *pdu)
 {
@@ -567,6 +581,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 		}
 
 		usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
+		stub_fixup_urb_number_of_packets(priv->urbs[0], np);
 	} else {
 		for_each_sg(sgl, sg, nents, i) {
 			priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
@@ -579,6 +594,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 			usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
 			priv->urbs[i]->transfer_buffer = sg_virt(sg);
 			priv->urbs[i]->transfer_buffer_length = sg->length;
+			stub_fixup_urb_number_of_packets(priv->urbs[i], 0);
 		}
 		priv->sgl = sgl;
 	}
-- 
2.55.0