Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove MAX_CTRL_BUFFER_LENGTH Limitation From Linux Build #1359

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Iskuri
Copy link

@Iskuri Iskuri commented Dec 3, 2023

Both the Linux and Windows versions of LibUSB have a limitation placed on the size of Control Transfers that can be sent using LibUSB, returning a LIBUSB_ERROR_INVALID_PARAM error if the size of the buffer exceeds 4096 bytes. This is an artificial limitation, as most modern hardware is capable of handling control transfer buffers up to the 65535 byte limit, and in Linux specifically there is nothing in the Kernel to reject larger buffers. I have created this patch specifically for Linux, however it would likely also be viable for Windows.

The presence of this limitation means that users who wish to use LibUSB to identify vulnerabilities in USB devices, or communicate with devices which permit buffer sizes of greater than 4096 bytes, will be presented with an error message that they would have to manually remove from the library. This patch simply removes the limitation, preventing the library from throwing a LIBUSB_ERROR_INVALID_PARAM error when >4096 bytes are used in control transfers on Linux builds.

@mcuee mcuee added the linux label Dec 3, 2023
@mcuee
Copy link
Member

mcuee commented Dec 3, 2023

@mcuee
Copy link
Member

mcuee commented Dec 3, 2023

Refer to mailing list answer from Alan Stern, one of the leading Linux USB developer.
https://sourceforge.net/p/libusb/mailman/message/33339533/

+++++++++++++++
Re: [libusb] Large control data stage
[Hide message from public view]
From: Alan S. <st...@ro...> - 2015-02-03 20:04:10
...
The Linux and Windows implementations of libusb both limit control
transfers to 4096 bytes (MAX_CTRL_BUFFER_LENGTH in
libusb/os/linux_usbfs.h and windows_usb.h in the libusb source).

At least part of the reason for this is that some of the Linux host
controller drivers can't handle control transfers larger than that.
If you need to send more data, you can break it up into multiple
transfers.

Alan Stern

@mcuee
Copy link
Member

mcuee commented Dec 3, 2023

As libusb is widely used and we do not now the user base well enough, for example, some embedded Linux distro may still have the same limitation.

Therefore I do not think we can take this PR.

@mcuee
Copy link
Member

mcuee commented Dec 4, 2023

For Windows, the size limit is documented by Microsoft. libusb dropped the support of Windows XP, but Windows Vista/7/8/8.1 are still supported (not so sure if anyone still cares about Windows Vista though).
https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/usb-bandwidth-allocation

64K for SuperSpeed and high speed (xHCI)
4K for full and low speed (xHCI, EHCI, UHCI, OHCI)
For UHCI, 4K on the default endpoint; 64K on non-default control pipes

@mcuee
Copy link
Member

mcuee commented Dec 5, 2023

@tormodvolden

I think we can close this PR.

@RReverser
Copy link
Contributor

RReverser commented Dec 5, 2023

At least part of the reason for this is that some of the Linux host
controller drivers can't handle control transfers larger than that.
If you need to send more data, you can break it up into multiple
transfers.

What happens if you send more data than the OS / controller supports? Wouldn't you receive an error from the backend anyway, even without this check in libusb itself?

@diabolo38
Copy link

even you device support more than 4K you'r application shall never assume what it can get you can't assume what is Host or hub bus speed capability you end up pluged into .
If you behind an FS speed hub then you'll fall in 4K OS limitation

Anyway control transfer are bad they're meant for system control not application data exchange it's a waste of bus bandwidth and it is jeopardizing ep0 usage on composite device.

@hjelmn
Copy link
Member

hjelmn commented Dec 8, 2023

I agree. We should probably close this. 4k is plenty for control transfers.

@RReverser
Copy link
Contributor

RReverser commented Dec 8, 2023

We should probably close this. 4k is plenty for control transfers.

To me, I don't think it's about "how much you need" but rather about erroring too early and in a platform-specific way.

It's something I ran into before as well - if I don't know how much data to expect, I'm happy to just give the OS a larger buffer, all the way up to 64K bytes (uint16 max, maximum possible for transfer_length) for reading data. The return transfer data will usually be much smaller, but that's not a problem, the main thing is that data will fit in such buffer on any platform.

But, libusb errors out early due to transfer_length being larger than it knows Linux / Windows will allow, so now users need to hardcode a platform-specific limit of 4096 bytes in their application. This is not ideal IMO.

If libusb knows that the OS doesn't support buffers larger than 4096 bytes, I think a better developer experience would be to clamp the transfer length to those 4096 bytes rather than return an error. If the device ever returns more data, then user will get the actual "babble" status, but if not - and, as you noted, usually it should be enough - then passing larger buffers won't be an error but the buffer simply won't be filled to its full capacity, like with any other transfers.

This would allow to better abstract out OS-specific limits from the high-level libusb API.

@RReverser
Copy link
Contributor

TL;DR instead of just removing this condition altogether, I'd clamp the transfer length for "in" transfers to the OS-specific limit. This way you still get errors if the device genuinely happens to return more data than the OS allows, and no errors otherwise.

@Youw
Copy link
Member

Youw commented Dec 14, 2023

@mcuee, @hjelmn what if we make this limitation a runtime variable?
I.e. default it to 4k as it is not (so we fully preserve current behavior) but add another LIBUSB option to override it in case if user wants to try to "break" something in this specific way.

@RReverser has a point that sometime one simply wants to laverage a platform-specific behavior.

@mcuee
Copy link
Member

mcuee commented Dec 18, 2023

@mcuee, @hjelmn what if we make this limitation a runtime variable? I.e. default it to 4k as it is not (so we fully preserve current behavior) but add another LIBUSB option to override it in case if user wants to try to "break" something in this specific way.

@RReverser has a point that sometime one simply wants to laverage a platform-specific behavior.

It seems to be a good improvement.

@hjelmn and @tormodvolden

How do you like this idea from @Youw?

@RReverser
Copy link
Contributor

RReverser commented Dec 18, 2023

I.e. default it to 4k as it is not (so we fully preserve current behavior) but add another LIBUSB option to override it in case if user wants to try to "break" something in this specific way.

FWIW for me the more important part is clamping instead of erroring as mentioned above. Without it, code using libusb is not really platform-agnostic as it needs to know on which platform it's running and hardcode those limits at the application level.

That is, re: "has a point that sometime one simply wants to laverage a platform-specific behavior" for me it's kinda the opposite, I want to avoid platform-specific behaviour to leak into apps.

Custom limits alone don't help with this particular issue much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants