On Mon, 16 Oct 2017, Hans de Goede wrote:
> Taking the uurb->buffer_length userspace passes in as a maximum for the
> actual urbs transfer_buffer_length causes 2 serious issues:
>
> 1) It breaks isochronous support for all userspace apps using libusb,
> as existing libusb versions pass in 0 for uurb->buffer_length,
> relying on the kernel using the lenghts of the usbdevfs_iso_packet_desc
> descriptors passed in added together as buffer length.
>
> This for example causes redirection of USB audio and Webcam's into
> virtual machines using qemu-kvm to no longer work. This is a userspace
> ABI break and as such must be reverted.
>
> Note that the original commit does not protect other users / the
> kernels memory, it only stops the userspace process making the call
> from shooting itself in the foot.
Okay, breaking userspace is reason enough all by itself to revert this
change. I didn't realize that libusb sets the buffer_length to 0 for
isochronous URBs.
> 2) It may cause the kernel to program host controllers to DMA over random
> memory. Just as the devio code used to only look at the iso_packet_desc
> lenghts, the host drivers do the same, relying on the submitter of the
> urbs to make sure the entire buffer is large enough and not checking
> transfer_buffer_length.
>
> But the "USB: devio: Don't corrupt user memory" commit now takes the
> userspace provided uurb->buffer_length for the buffer-size while copying
> over the user-provided iso_packet_desc lengths 1:1, allowing the user
> to specify a small buffer size while programming the host controller to
> dma a lot more data.
Wow! You're right, of course. I don't know what I was thinking when I
reviewed that patch.
> (Atleast the ohci, uhci, xhci and fhci drivers do not check
> transfer_buffer_length for isoc transfers.)
>
> This reverts commit fa1ed74eb1c2 ("USB: devio: Don't corrupt user memory")
> fixing both these issues.
>
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/usb/core/devio.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 4664e543cf2f..e9326f31db8d 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -1576,11 +1576,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> totlen += isopkt[u].length;
> }
> u *= sizeof(struct usb_iso_packet_descriptor);
> - if (totlen <= uurb->buffer_length)
> - uurb->buffer_length = totlen;
> - else
> - WARN_ONCE(1, "uurb->buffer_length is too short %d vs %d",
> - totlen, uurb->buffer_length);
> + uurb->buffer_length = totlen;
> break;
>
> default:
For what it's worth:
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>