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

[sashiko-bot@kernel.org]

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