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

[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

[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 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 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

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

[Cheng Yongkang]

Hi Shuah, Sung,

My patch 1/2 makes the same change as Sung's earlier submission:

https://lore.kernel.org/linux-usb/20261009122636.642575-1-tjdqudcks0424@naver.com/

Please drop mine; sorry for the duplicate. My explanation above of why
-EPROTO alone does not stop the usbfs copy applies to Sung's patch as
well.

That change does not cover the high-bit length case raised in the
review of Sung's patch:

https://lore.kernel.org/linux-usb/sashiko-outbox-165560@kernel.org/

I have only checked this in the source: usbip_recv_xbuff() stores the
length in an int, so a value with the top bit set takes the size <= 0
early return, and the URB is given back with that length.

I have sent the usbfs side separately. It bounds actual_length in
usbfs regardless of what the HCD reports:

https://lore.kernel.org/linux-usb/20261101070651.1846947-1-teel4res@gmail.com/

Thanks,
Cheng Yongkang

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

[Shuah Khan]

On 10/29/26 22:13, Cheng Yongkang wrote:
> 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:

It sounds like fixing usbfs to check the URB status before copying
sounds like the right fix.

Changing vhci to zero out a filed in an URB that it received, itsn't
something we want to do. Why not fix this in usbfs and that would work
for other invalid lengths?

thanks,
-- Shuah

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

[Alan Stern]

On Tue, Nov 10, 2026 at 11:57:34AM -0700, Shuah Khan wrote:
> On 10/29/26 22:13, Cheng Yongkang wrote:
> > 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:
> 
> It sounds like fixing usbfs to check the URB status before copying
> sounds like the right fix.

No, that's not possible.  A transfer may terminate with an error partway 
through, after some data has been sent/received.  The URB status would 
then be a negative error code, but actual_length would still need to 
reflect the amount of data that usbfs has to copy back to userspace (for 
an IN transfer) or tell userspace was sent (for an OUT transfer).

In general, the USB core relies on HCDs to set urb->actual_length 
correctly even when there was an error.

> Changing vhci to zero out a filed in an URB that it received, itsn't
> something we want to do. Why not fix this in usbfs and that would work
> for other invalid lengths?

HCDs are _supposed_ to set the actual_length field in URBs.  It's not 
a mistake to do so.

Alan Stern

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

[Shuah Khan]

On 11/10/26 14:29, Alan Stern wrote:
> On Tue, Nov 10, 2026 at 11:57:34AM -0700, Shuah Khan wrote:
>> On 10/29/26 22:13, Cheng Yongkang wrote:
>>> 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:
>>
>> It sounds like fixing usbfs to check the URB status before copying
>> sounds like the right fix.

Cheng, I missed that the you sent usbfs patch to bound the length. Thank
you for both patches.

> 
> No, that's not possible.  A transfer may terminate with an error partway
> through, after some data has been sent/received.  The URB status would
> then be a negative error code, but actual_length would still need to
> reflect the amount of data that usbfs has to copy back to userspace (for
> an IN transfer) or tell userspace was sent (for an OUT transfer).
> 
> In general, the USB core relies on HCDs to set urb->actual_length
> correctly even when there was an error.
> 
>> Changing vhci to zero out a filed in an URB that it received, itsn't
>> something we want to do. Why not fix this in usbfs and that would work
>> for other invalid lengths?
> 
> HCDs are _supposed_ to set the actual_length field in URBs.  It's not
> a mistake to do so.

Alan, Thank you for weighing on this.

Greg, please pick this patch up.

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

thanks,
-- Shuah