usbip: vhci: do not give back URBs with an unchecked actual_length
Subsystems (2)
PATCH 1/1 usbip: vhci: do not give back URBs with an unchecked actual_length
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.
---
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(+)
drivers/usb/usbip/vhci_rx.c +8 -0
@@ -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
[email protected]
AI reviewThank 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 --
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 <[email protected]>
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.
Quoted 9 lines
drivers/usb/usbip/vhci_rx.c +1
-0@@ -78,6 +78,14 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,/* recv transfer buffer */ if (usbip_recv_xbuff (ud, urb) <0 ) { +/*
drivers/usb/usbip/vhci_rx.c static void vhci_recv_ret_submit(struct vhci_device *vdev, · line 91
Quoted 10 lines
drivers/usb/usbip/vhci_rx.c +7
-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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 2 lines · On Wed, Nov 04, 2026 at 06:16:14AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usbip: vhci: negative wireactual_lengthbypassesusbip_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