[PATCH 0/2] usb: fix over-reported actual_length in usbip/usbfs

[PATCH 0/2] usb: fix over-reported actual_length in usbip/usbfs

[Cheng Yongkang]

The usbip vhci receive error path gives a failed URB back with the
peer-supplied actual_length, and usbfs copy_urb_data_to_user() trusts
that length for non-isochronous URBs.  A malicious usbip peer can make
usbfs read past the transfer buffer.

Patch 1 clears actual_length on the vhci receive failure path.  Patch 2
clamps the copied length to transfer_buffer_length in usbfs as a
defense-in-depth measure for any HCD that violates the actual_length
contract.

Cheng Yongkang (2):
  usbip: clear actual_length after a failed receive
  usb: usbfs: clamp the copied length to transfer_buffer_length

 drivers/usb/core/devio.c    | 7 +++++--
 drivers/usb/usbip/vhci_rx.c | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] usbip: clear actual_length after a failed receive

[Cheng Yongkang]

vhci_recv_ret_submit() receives the transfer buffer for an IN URB.  When
usbip_recv_xbuff() fails, it sets urb->status to -EPROTO and gives the URB
back, but leaves urb->actual_length at the value supplied by the usbip
peer.  A malicious peer can set actual_length larger than
transfer_buffer_length; usbfs then copies that many bytes from the
kmalloc(transfer_buffer_length) buffer to userspace:

  BUG: KASAN: slab-out-of-bounds in _copy_to_user+0x79/0xb0
  Read of size 88 at addr ...
  copy_urb_data_to_user+0x36f/0x480

Clear actual_length on the failure path so the failed URB never carries a
peer-controlled length.  The URB is completed with -EPROTO and userspace
observes a zero-length, failed transfer.

Fixes: aabb5b833872 ("usbip: Fix error path of vhci_recv_ret_submit()")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Cheng Yongkang <teel4res@gmail.com>
---
 drivers/usb/usbip/vhci_rx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index a678e7c..741107a 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -79,6 +79,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 	/* recv transfer buffer */
 	if (usbip_recv_xbuff(ud, urb) < 0) {
 		urb->status = -EPROTO;
+		urb->actual_length = 0;
 		goto error;
 	}
 
-- 
2.34.1

Re: [PATCH 1/2] usbip: clear actual_length after a failed receive

[sashiko-bot]

> vhci_recv_ret_submit() receives the transfer buffer for an IN URB.  When
> usbip_recv_xbuff() fails, it sets urb->status to -EPROTO and gives the URB
> back, but leaves urb->actual_length at the value supplied by the usbip
> peer.  A malicious peer can set actual_length larger than
> transfer_buffer_length; usbfs then copies that many bytes from the
> kmalloc(transfer_buffer_length) buffer to userspace:
> 
>   BUG: KASAN: slab-out-of-bounds in _copy_to_user+0x79/0xb0
>   Read of size 88 at addr ...
>   copy_urb_data_to_user+0x36f/0x480
> 
> Clear actual_length on the failure path so the failed URB never carries a
> peer-controlled length.  The URB is completed with -EPROTO and userspace
> observes a zero-length, failed transfer.
> 
> Fixes: aabb5b833872 ("usbip: Fix error path of vhci_recv_ret_submit()")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Cheng Yongkang <teel4res@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 1/2] usbip: clear actual_length after a failed receive

[Shuah Khan]

On 10/29/26 08:12, Cheng Yongkang wrote:
> vhci_recv_ret_submit() receives the transfer buffer for an IN URB.  When
> usbip_recv_xbuff() fails, it sets urb->status to -EPROTO and gives the URB
> back, but leaves urb->actual_length at the value supplied by the usbip
> peer.  A malicious peer can set actual_length larger than
> transfer_buffer_length; usbfs then copies that many bytes from the
> kmalloc(transfer_buffer_length) buffer to userspace:
> 
>    BUG: KASAN: slab-out-of-bounds in _copy_to_user+0x79/0xb0
>    Read of size 88 at addr ...
>    copy_urb_data_to_user+0x36f/0x480
> 
> Clear actual_length on the failure path so the failed URB never carries a
> peer-controlled length.  The URB is completed with -EPROTO and userspace
> observes a zero-length, failed transfer.
> 
> Fixes: aabb5b833872 ("usbip: Fix error path of vhci_recv_ret_submit()")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Cheng Yongkang <teel4res@gmail.com>
> ---
>   drivers/usb/usbip/vhci_rx.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> index a678e7c..741107a 100644
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -79,6 +79,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
>   	/* recv transfer buffer */
>   	if (usbip_recv_xbuff(ud, urb) < 0) {
>   		urb->status = -EPROTO;
> +		urb->actual_length = 0;
>   		goto error;
>   	}
>   

What happens without this code change? The code path follows the
error leg because of EPROTO? So how does setting actual_length to
zero help?

thanks,
-- Shuah

Re: [PATCH 1/2] usbip: clear actual_length after a failed receive

[Cheng Yongkang]

Hi Shuah,

Without this change the URB is still completed to usbfs, with status
-EPROTO but with the peer-supplied actual_length intact:

  vhci_recv_ret_submit()
    usbip_pack_pdu() -> usbip_pack_ret_submit():
        urb->actual_length = rpdu->actual_length;   (usbip_common.c:471)
    usbip_recv_xbuff():
        size = urb->actual_length;                  (usbip_common.c:849)
        if (size > urb->transfer_buffer_length)     (usbip_common.c:856)
                goto error;  /* rejects the packet, returns < 0 */
    urb->status = -EPROTO; goto error;
  error:
    usb_hcd_giveback_urb(hcd, urb, urb->status);    (vhci_rx.c:107)
      -> urb->complete(urb)                         (hcd.c:1657)
      -> usbfs async_completed()                    (devio.c:1911)

usbfs copies data on reap whenever actual_length is non-zero, and does
not look at the URB status:

  processcompl():
    if (as->userbuffer && urb->actual_length)       (devio.c:2050)
        copy_urb_data_to_user(as->userbuffer, urb);
  copy_urb_data_to_user():
    len = urb->actual_length;  /* non-isochronous */ (devio.c:563)

That is deliberate: a legitimate HCD can complete with a short read and
a non-zero status, and userspace still gets the partial data.  But here
the length came from the peer and was already rejected as invalid, so it
must not reach usbfs.  -EPROTO alone does not stop the copy; zeroing
actual_length makes the copy condition false and userspace observes a
failed, zero-length transfer.

I'll fold this into the commit message for the next version.

Thanks,
Cheng Yongkang

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

[Cheng Yongkang]

copy_urb_data_to_user() uses urb->actual_length as the number of bytes to
copy to userspace for non-isochronous URBs.  The usbfs contract is that a
driver must not report more data than the transfer buffer holds, but
usbfs cannot rely on every HCD or device implementation to honor it; a
malicious usbip peer can already set actual_length beyond
transfer_buffer_length and trigger an out-of-bounds read:

  BUG: KASAN: slab-out-of-bounds in _copy_to_user+0x79/0xb0
  Read of size 88 at addr ...
  copy_urb_data_to_user+0x36f/0x480

Clamp the non-isochronous length to transfer_buffer_length before the
copy.  Normal URBs are unaffected; the isochronous path already uses
transfer_buffer_length.

Fixes: 3d97ff63f899 ("usbdevfs: Use scatter-gather lists for large bulk transfers")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Cheng Yongkang <teel4res@gmail.com>
---
 drivers/usb/core/devio.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 101cb94..9554b6a 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;
+	}
 
 	if (urb->num_sgs == 0) {
 		if (copy_to_user(userbuffer, urb->transfer_buffer, len))
-- 
2.34.1

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

[sashiko-bot]

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