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

[Shuah Khan <skhan@linuxfoundation.org>]

On 11/5/26 01:10, Vlatko Kosturjak wrote:
> vhci_recv_ret_submit() copies actual_length from the RET_SUBMIT header
> into the URB, and only usbip_recv_xbuff() looks at it. That check is not
> enough:
> 
>   - an IN reply longer than transfer_buffer_length is rejected and the
>     connection shut down, but the URB is still given back with the
>     oversized actual_length (and status -EPROTO);
>   - a negative actual_length (urb->actual_length is a u32) is not
>     "> 0", so usbip_recv_xbuff() returns 0 without checking it and the
>     URB is given back with status 0;
>   - for OUT URBs usbip_recv_xbuff() returns 0 straight away, so any
>     actual_length is given back.
> 
> 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, here for a 22 byte reply to a 16 byte
> 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. A
> negative actual_length does the same on a URB that usbfs splits into
> scatterlist chunks: the last, shorter chunk is over-read by up to 16 KiB.
> 
> 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.
> 
> Check actual_length against transfer_buffer_length right after
> unpacking the header, for IN and OUT URBs: the u32 comparison also
> catches negative values. On failure give the URB back with
> actual_length 0 and status -EPROTO and shut the connection down, as
> usbip_recv_xbuff() does, since any payload was not read. Also give the
> URB back with actual_length 0 when receiving the transfer buffer fails
> (a short read), as the buffer was not filled.
> 
> Fixes: b348d7dddb6c ("USB: usbip: fix potential out-of-bounds write")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vlatko Kosturjak <kost@linux.hr>
> ---
> Changes in v2:
>   - Check actual_length before usbip_recv_xbuff() instead of only zeroing
>     it when usbip_recv_xbuff() fails. v1 missed a negative actual_length
>     (usbip_recv_xbuff() skips it, so the URB was given back with status 0)
>     and OUT URBs (never checked). Pointed out by the Sashiko AI review.
>   - Shut the connection down on that error, as usbip_recv_xbuff() does.
>   - Tested on mainline 7.4-rc1 as well.
> 
> v1: https://lore.kernel.org/linux-usb/aurLXADfuLjs-dm-@griffin.linux.hr/
> 
> Tested in QEMU on mainline 7.4-rc1 (05441d697f7f, x86_64 defconfig +
> kvm_guest.config + USBIP_VHCI_HCD=m, HARDENED_USERCOPY=y) and on 7.0
> (Ubuntu 7.0.0-34-generic), with the same results on both. The reproducer
> is a user-space program that attaches a fake device to vhci_hcd through
> a Unix socketpair (no network or usbip tools needed), answers a bulk URB
> with a bad actual_length, and reaps the URB through usbfs:
> 
>    reply                         unpatched / v1              v2
>    IN 16 bytes, length 22        BUG (v1: -EPROTO, length 0) -EPROTO, length 0
>    IN 16 bytes, length -22       WARN in check_copy_size(),  -EPROTO, length 0
>                                  REAPURB fails with EFAULT
>    IN 16400 bytes, length -22    BUG; with hardened_usercopy -EPROTO, length 0
>    (two usbfs sg chunks)         =off status 0 and 16 KiB
>                                  of heap reach user space
>    OUT 16 bytes, length 22       status 0, length 22         -EPROTO, length 0
> 
> With hardened_usercopy=off the leaked heap bytes include kernel
> pointers. Legitimate replies are unaffected: device enumeration over the
> same connection works as before.
> 
> Happy to send the reproducer (about 350 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 | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> index a678e7c898..3180dc3018 100644
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -76,8 +76,27 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
>   	/* unpack the pdu to a urb */
>   	usbip_pack_pdu(pdu, urb, USBIP_RET_SUBMIT, 0);
>   
> +	/*
> +	 * actual_length comes from the server, and completion handlers
> +	 * trust it: usbfs copies actual_length bytes out of transfer_buffer.
> +	 * Reject a negative one (it is a u32 in the URB) or one larger than
> +	 * the buffer, for IN and OUT URBs alike. Any payload was not read,
> +	 * so the stream is out of sync: drop the connection.
> +	 */
> +	if (urb->actual_length > urb->transfer_buffer_length) {
> +		dev_err(&urb->dev->dev, "ret_submit: actual_length %d exceeds buffer %u\n",
> +			pdu->u.ret_submit.actual_length,
> +			urb->transfer_buffer_length);
> +		urb->actual_length = 0;
> +		urb->status = -EPROTO;
> +		usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> +		goto error;
> +	}

I will take version of this patch with just the above change.
> +
>   	/* recv transfer buffer */
>   	if (usbip_recv_xbuff(ud, urb) < 0) {
> +		/* short read: the buffer was not filled */
> +		urb->actual_length = 0;
>   		urb->status = -EPROTO;
>   		goto error;
>   	}

This fix will go in a different patch.

https://lore.kernel.org/r/179328314074.648427.388274461758949120@gmail.com

thanks,
-- Shuah