[PATCH] usbip: vhci: do not give back URBs with an unchecked actual_length

[PATCH] usbip: vhci: do not give back URBs with an unchecked actual_length

[Vlatko Kosturjak]

vhci_recv_ret_submit() copies actual_length from the RET_SUBMIT header
into the URB before usbip_recv_xbuff() checks it. When the server reports
more data than the URB's transfer buffer holds, usbip_recv_xbuff() rejects
the reply and shuts the connection down, but the URB is still given back
with the oversized actual_length (and status -EPROTO).

Completion handlers trust actual_length. usbfs copies that many bytes from
the transfer buffer to user space in copy_urb_data_to_user(), whatever the
URB status, so the kernel reads past the kmalloc()ed buffer. With
CONFIG_HARDENED_USERCOPY this is a BUG in the context of the process
reaping the URB:

  usb 1-1: recv xbuf, 0
  usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-rnd-05-16' (offset 0, size 22)!
  kernel BUG at mm/usercopy.c:102!
  RIP: 0010:usercopy_abort+0x6c/0x80
  Call Trace:
   copy_urb_data_to_user+0x122/0x160
   processcompl+0xe3/0x160
   usbdev_do_ioctl+0x14d/0x1900

Without it, the adjacent heap contents are copied to user space.

This was hit with a buggy user-space USB/IP device implementation (it
answered an IN URB with more data than requested) while pcscd read from
the device; any server, buggy or malicious, can trigger it.

Do not trust actual_length when the transfer buffer could not be
received: give the URB back with actual_length 0. A short read leaves the
buffer partly unfilled too, so this covers both failure cases.

Fixes: b348d7dddb6c ("USB: usbip: fix potential out-of-bounds write")
Cc: stable@vger.kernel.org
Signed-off-by: Vlatko Kosturjak <kost@linux.hr>
---
Tested on host 7.0 (Ubuntu 7.0.0-34-generic) in QEMU with a small reproducer: a
user-space program that attaches a fake device to vhci_hcd through a Unix
socketpair (no network or usbip tools needed), answers a 16-byte bulk IN
URB with actual_length 22, and reaps the URB through usbfs:

  unpatched, CONFIG_HARDENED_USERCOPY=y:  the BUG above
  unpatched, hardened_usercopy=off:       reaped actual_length 22 for a 16-byte
                                          buffer; with 200 extra bytes, 200 bytes
                                          of adjacent heap reach user space
  patched:                                reaped status -EPROTO, actual_length 0

The vhci_rx.c hunk is the same in v7.0 and in mainline. Happy to send the
reproducer (about 300 lines of C plus a QEMU script) if useful.

The 2026 hardening of RET_SUBMIT (number_of_packets, iso frame lengths)
does not cover this path; it is the non-isochronous actual_length.

 drivers/usb/usbip/vhci_rx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index a678e7c89..720dafa72 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -78,6 +78,14 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 
 	/* recv transfer buffer */
 	if (usbip_recv_xbuff(ud, urb) < 0) {
+		/*
+		 * The transfer buffer was not filled: either a short read or
+		 * an actual_length larger than transfer_buffer_length, which
+		 * usbip_recv_xbuff() rejects. Do not give the URB back with
+		 * that actual_length: completion handlers trust it, e.g.
+		 * usbfs copies actual_length bytes out of transfer_buffer.
+		 */
+		urb->actual_length = 0;
 		urb->status = -EPROTO;
 		goto error;
 	}

base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d

Re: [PATCH] usbip: vhci: do not give back URBs with an unchecked actual_length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usbip: vhci: negative wire actual_length bypasses usbip_recv_xbuff() check
--

commit e0d5d4fc3a36398ed4d68d108185b0a82c1a635b
Author: Vlatko Kosturjak <kost@linux.hr>

usbip: vhci: do not give back URBs with an unchecked actual_length

This commit zeroes urb->actual_length in vhci_recv_ret_submit() when
usbip_recv_xbuff() fails so completion handlers such as usbfs do not read past
urb->transfer_buffer.

> diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> index a678e7c898375..720dafa72f7e1 100644
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -78,6 +78,14 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
>  
>  	/* recv transfer buffer */
>  	if (usbip_recv_xbuff(ud, urb) < 0) {
> +		/*
> +		 * The transfer buffer was not filled: either a short read or
> +		 * an actual_length larger than transfer_buffer_length, which
> +		 * usbip_recv_xbuff() rejects. Do not give the URB back with
> +		 * that actual_length: completion handlers trust it, e.g.
> +		 * usbfs copies actual_length bytes out of transfer_buffer.
> +		 */
> +		urb->actual_length = 0;
>  		urb->status = -EPROTO;
>  		goto error;
>  	}

[Severity: Critical]
Does relying on usbip_recv_xbuff() < 0 here still allow a server to pass an
out-of-bounds urb->actual_length to usb_hcd_giveback_urb() when the wire
actual_length is negative (0x80000000..0xffffffff) or when the URB is an OUT
transfer?

Before calling usbip_recv_xbuff(), vhci_recv_ret_submit() calls
usbip_pack_pdu() -> usbip_pack_ret_submit(), which copies the signed __s32
rpdu->actual_length directly into the unsigned u32 urb->actual_length:

drivers/usb/usbip/usbip_common.c:usbip_pack_ret_submit() {
	...
	if (pack) {
		...
	} else {
		urb->status		= rpdu->status;
		urb->actual_length	= rpdu->actual_length;
		urb->start_frame	= rpdu->start_frame;
		urb->number_of_packets	= rpdu->number_of_packets;
		urb->error_count	= rpdu->error_count;
	}
}

Then usbip_recv_xbuff() assigns urb->actual_length into a signed int size and
returns 0 before the bounds check if usb_pipeout(urb->pipe) is true or if
!(size > 0):

drivers/usb/usbip/usbip_common.c:usbip_recv_xbuff() {
	...
	} else {
		/* the direction of urb must be IN. */
		if (usb_pipeout(urb->pipe))
			return 0;

		size = urb->actual_length;
	}

	/* no need to recv xbuff */
	if (!(size > 0))
		return 0;

	if (size > urb->transfer_buffer_length)
		/* should not happen, probably malicious packet */
		goto error;
	...
}

If a server replies to an IN URB with a negative rpdu->actual_length (for
example -22, which becomes 0xffffffea in u32 urb->actual_length), size is
negative in usbip_recv_xbuff(), so !(size > 0) is true and usbip_recv_xbuff()
returns 0 without reading from the socket or signalling an error.

vhci_recv_ret_submit() then gives the URB back with urb->actual_length >=
0x80000000, and processcompl() -> copy_urb_data_to_user() treats
urb->actual_length as an unsigned byte count:

drivers/usb/core/devio.c:copy_urb_data_to_user() {
	...
	if (urb->number_of_packets > 0)		/* Isochronous */
		len = urb->transfer_buffer_length;
	else					/* Non-Isoc */
		len = urb->actual_length;

	if (urb->num_sgs == 0) {
		if (copy_to_user(userbuffer, urb->transfer_buffer, len))
			return -EFAULT;
		return 0;
	}
	...
}

That still reads past urb->transfer_buffer (or copies uninitialized
scatterlist buffers when urb->num_sgs > 0) and triggers the same usercopy
check. Similarly, for an OUT URB, usbip_recv_xbuff() returns 0 immediately on
usb_pipeout(urb->pipe), leaving an oversized or negative rpdu->actual_length
unchecked in urb->actual_length.

Should vhci_recv_ret_submit() (or usbip_recv_xbuff()) validate that
rpdu->actual_length >= 0 and urb->actual_length <= urb->transfer_buffer_length
for both IN and OUT URBs before giving the URB back?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/aurLXADfuLjs-dm-@griffin.linux.hr?part=1

Re: [PATCH] usbip: vhci: do not give back URBs with an unchecked actual_length

[Vlatko Kosturjak]

On Wed, Nov 04, 2026 at 06:16:14AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] usbip: vhci: negative wire actual_length bypasses usbip_recv_xbuff() check

The review is right: a negative actual_length (usbip_recv_xbuff() skips
it and the URB is given back with status 0) and OUT URBs (never checked)
are not covered by v1. I reproduced both on 7.4-rc1; with a URB that
usbfs splits into scatterlist chunks, a negative length over-reads the
last chunk by up to 16 KiB.

Please don't apply v1. v2 checks actual_length against
transfer_buffer_length right after unpacking the header, for IN and OUT;
I'll send it after giving v1 a day for other comments.

--
Vlatko Kosturjak - KoSt