[PATCH v2] usb: usbfs: clamp actual_length to transfer_buffer_length

[PATCH v2] usb: usbfs: clamp actual_length to transfer_buffer_length

[Cheng Yongkang]

usbfs uses urb->actual_length from the host controller driver without
checking it against the buffer it allocated. For a completed
non-isochronous IN URB, processcompl() and processcompl_compat() copy
that many bytes to userspace and report the same value, and
async_completed() passes it to snoop_urb_data() when usbfs_snoop is
enabled.

With USB/IP, the value comes from the remote peer. If the peer returns
actual_length=88 for a 24-byte control-IN transfer, usbip_recv_xbuff()
rejects the length, but VHCI still gives back the URB with -EPROTO and
actual_length=88. usbfs copies data from failed URBs too, since they
can carry valid partial data, so reaping the URB reads past the buffer:

  BUG: KASAN: slab-out-of-bounds in _copy_to_user
   copy_urb_data_to_user

With usbfs_snoop enabled, hex_dump_to_buffer() reads past the buffer
earlier, when the URB completes.

Clamp urb->actual_length in async_completed() while holding ps->lock,
before the completion is snooped or can be reaped. The copy, the
reported length and the snoop dump then stay within
transfer_buffer_length.

Tested on usb-linus e971f4f365ba with KASAN, using a synthetic USB/IP
peer that returns 88 bytes for a 24-byte control-IN transfer. Without
this change, reaping the URB hits the report above, and with
usbfs_snoop enabled the completion hits it in hex_dump_to_buffer().
With it, the URB is reaped with -EPROTO and actual_length=24 in both
cases. Full and short control-IN and control-OUT transfers through
usbfs still work.

Fixes: 0880aef49e40a ("USB: usbfs_snoop: add data logging back in")
Fixes: d4a4683ca054e ("USB: usbfs: only copy the actual data received")
Cc: stable@vger.kernel.org
Signed-off-by: Cheng Yongkang <teel4res@gmail.com>
---
Changes in v2:
- Clamp urb->actual_length in async_completed() instead of only the
  copy length in copy_urb_data_to_user(), so the reported length and
  the snoop dump are bounded too, as pointed out in review [2].
- Fix the Fixes tags; v1 named a later scatter-gather refactor.
- Send separately. The USB/IP patch in v1 duplicates Sung Byeongchan's
  earlier submission [1], which covers only the VHCI receive-failure
  path, so usbfs still needs its own bound.

v1: https://lore.kernel.org/linux-usb/179328314074.648427.8187049654053191605@gmail.com/
[1]: https://lore.kernel.org/linux-usb/20261009122636.642575-1-tjdqudcks0424@naver.com/
[2]: https://lore.kernel.org/linux-usb/sashiko-outbox-189547@kernel.org/

 drivers/usb/core/devio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 101cb9425..7319e8b98 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -633,6 +633,8 @@ static void async_completed(struct urb *urb)
 	spin_lock_irqsave(&ps->lock, flags);
 	list_move_tail(&as->asynclist, &ps->async_completed);
 	as->status = urb->status;
+	if (urb->actual_length > urb->transfer_buffer_length)
+		urb->actual_length = urb->transfer_buffer_length;
 	signr = as->signr;
 	if (signr) {
 		errno = as->status;

base-commit: e971f4f365bad51d01bc90db55b56fdb3124f963
-- 
2.34.1

Re: [PATCH v2] usb: usbfs: clamp actual_length to transfer_buffer_length

[sashiko-bot]

> usbfs uses urb->actual_length from the host controller driver without
> checking it against the buffer it allocated. For a completed
> non-isochronous IN URB, processcompl() and processcompl_compat() copy
> that many bytes to userspace and report the same value, and
> async_completed() passes it to snoop_urb_data() when usbfs_snoop is
> enabled.
> 
> With USB/IP, the value comes from the remote peer. If the peer returns
> actual_length=88 for a 24-byte control-IN transfer, usbip_recv_xbuff()
> rejects the length, but VHCI still gives back the URB with -EPROTO and
> actual_length=88. usbfs copies data from failed URBs too, since they
> can carry valid partial data, so reaping the URB reads past the buffer:
> 
>   BUG: KASAN: slab-out-of-bounds in _copy_to_user
>    copy_urb_data_to_user
> [ ... ]
> 
> Fixes: 0880aef49e40a ("USB: usbfs_snoop: add data logging back in")
> Fixes: d4a4683ca054e ("USB: usbfs: only copy the actual data received")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cheng Yongkang <teel4res@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v2] usb: usbfs: clamp actual_length to transfer_buffer_length

[Greg Kroah-Hartman]

On Sun, Nov 01, 2026 at 12:06:51AM -0700, Cheng Yongkang wrote:
> usbfs uses urb->actual_length from the host controller driver without
> checking it against the buffer it allocated. For a completed
> non-isochronous IN URB, processcompl() and processcompl_compat() copy
> that many bytes to userspace and report the same value, and
> async_completed() passes it to snoop_urb_data() when usbfs_snoop is
> enabled.
> 
> With USB/IP, the value comes from the remote peer. If the peer returns
> actual_length=88 for a 24-byte control-IN transfer, usbip_recv_xbuff()
> rejects the length, but VHCI still gives back the URB with -EPROTO and
> actual_length=88. usbfs copies data from failed URBs too, since they
> can carry valid partial data, so reaping the URB reads past the buffer:
> 
>   BUG: KASAN: slab-out-of-bounds in _copy_to_user
>    copy_urb_data_to_user
> 
> With usbfs_snoop enabled, hex_dump_to_buffer() reads past the buffer
> earlier, when the URB completes.

Why not fix this properly in usbip so that all drivers don't have to do
this type of check as well?  That's where the "bug" is, if any.

thanks,

greg k-h

Re: [PATCH v2] usb: usbfs: clamp actual_length to transfer_buffer_length

[Cheng Yongkang]

Hi Greg,

Agreed, that is where the bug is, so please drop this patch.

That was in fact the first fix: patch 1/2 of my original series cleared
actual_length in vhci when usbip_recv_xbuff() fails:

https://lore.kernel.org/linux-usb/179328314074.648427.388274461758949120@gmail.com/

I withdrew it only because Sung Byeongchan had posted the same one-liner
three weeks earlier:

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

That patch has been waiting since, with an open question from Shuah.
Its review had also already pointed out the case it misses: a returned
length with the top bit set is read into an int in usbip_recv_xbuff(),
takes the size <= 0 early return, and the URB is given back with that
length anyway:

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

Rejecting any actual_length larger than transfer_buffer_length in
vhci_recv_ret_submit() covers both. I can send that, or Sung can fold
it into his patch, whichever you and Shuah prefer.

Thanks,
Cheng Yongkang