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

[Shuah Khan <skhan@linuxfoundation.org>]

On 11/3/26 23:06, Vlatko Kosturjak wrote:
> 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;
>   	}

Greg, please pick this up.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah