Re: [PATCH 2/2] usb: usbfs: clamp the copied length to transfer_buffer_length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: usbfs: unclamped urb->actual_length reported in processcompl()
--

commit c0187fb6be71954b8fdc91f54fa38ce341728b34
Author: Cheng Yongkang <teel4res@gmail.com>

usb: usbfs: clamp the copied length to transfer_buffer_length

This commit clamps the local copy length in copy_urb_data_to_user() to
urb->transfer_buffer_length for non-isochronous URBs so that an HCD or
usbip peer reporting urb->actual_length larger than the transfer buffer
does not trigger an out-of-bounds read during copy_to_user().

> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 101cb94254809..9554b6a87a1f6 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -557,10 +557,13 @@ static int copy_urb_data_to_user(u8 __user *userbuffer, struct urb *urb)
>  {
>  	unsigned i, len, size;
>  
> -	if (urb->number_of_packets > 0)		/* Isochronous */
> +	if (urb->number_of_packets > 0) {	/* Isochronous */
>  		len = urb->transfer_buffer_length;
> -	else					/* Non-Isoc */
> +	} else {				/* Non-Isoc */
>  		len = urb->actual_length;
> +		if (len > urb->transfer_buffer_length)
> +			len = urb->transfer_buffer_length;
> +	}

[Severity: Medium]
Should urb->actual_length itself be clamped (for example on completion in
async_completed()), rather than only clamping the local len variable in
copy_urb_data_to_user()?

When a non-isochronous URB submitted via USBDEVFS_SUBMITURB completes with
urb->actual_length > urb->transfer_buffer_length and is reaped via
USBDEVFS_REAPURB or USBDEVFS_REAPURBNDELAY, copy_urb_data_to_user() copies
at most urb->transfer_buffer_length bytes into the user buffer, but
processcompl() and processcompl_compat() then report the unclamped
urb->actual_length to userspace:

drivers/usb/core/devio.c:processcompl() {
	if (as->userbuffer && urb->actual_length) {
		if (copy_urb_data_to_user(as->userbuffer, urb))
			goto err_out;
	}
	if (put_user(as->status, &userurb->status))
		goto err_out;
	if (put_user(urb->actual_length, &userurb->actual_length))
		goto err_out;
}

drivers/usb/core/devio.c:processcompl_compat() {
	if (as->userbuffer && urb->actual_length) {
		if (copy_urb_data_to_user(as->userbuffer, urb))
			return -EFAULT;
	}
	if (put_user(as->status, &userurb->status))
		return -EFAULT;
	if (put_user(urb->actual_length, &userurb->actual_length))
		return -EFAULT;
}

This tells userspace that userurb->actual_length bytes were transferred
when only urb->transfer_buffer_length bytes were copied into userurb->buffer,
which can cause userspace callers to read uninitialized or out-of-bounds
memory.

In addition, before the URB is reaped, async_completed() passes the
unclamped urb->actual_length to snoop_urb_data():

drivers/usb/core/devio.c:async_completed() {
	snoop_urb(urb->dev, as->userurb, urb->pipe, urb->actual_length,
			as->status, COMPLETE, NULL, 0);
	if (usb_urb_dir_in(urb))
		snoop_urb_data(urb, urb->actual_length);
}

When usbfs_snoop is enabled, can snoop_urb_data() still read past the end
of urb->transfer_buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179328314073.648427.8311262845412746237@gmail.com?part=2