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

usbd_ep_setup implementations write out of bounds for endpoint numbers > 7 #666

Closed
devanlai opened this issue Jun 27, 2016 · 3 comments
Closed

Comments

@devanlai
Copy link
Contributor

All existing USB driver implementations of ep_setup use the endpoint number to index into into the usbd_dev->user_callback_ctr callback table for endpoint handlers. user_callback_ctr has a hard coded size of 8, registering a callback for any endpoint number > 7 will cause an out of bounds write to other fields in the _usbd_device struct. Conveniently, registering a handler for endpoint address 0x88 will overwrite the second entry in usbd_dev->user_callback_set_config...

While there's no need to use such high endpoint numbers in most cases, the limitation should at least be documented, since endpoint numbers 8-15 are generally valid.

@karlp
Copy link
Member

karlp commented Jun 27, 2016

got any hardware that supports those endpoints? OTG_HS has 5 in plus 5 out. OTG_FS has 3 in +3 out, st-usbfs has 8 total. While, yes, you can use non-sequential endpoint numbers, there's no reason for it, and there's many reasons not to.

Definitely trampling over data structures is a pain, and docs can always be expanded, but there's no history or precedent of argument checking, and it's unlikely to ever be added. Do you have a particular place where you feel this "limitation" should be documented?

@devanlai
Copy link
Contributor Author

I was using non-sequential end-point numbers because it was convenient to map one endpoint number to each interface, with some interfaces omitted in different build configurations, hence endpoint numbers greater than 7, even if only up to 8 endpoints were actually used.
Before I looked into the USB driver code, there didn't seem to be any reason not to use the higher endpoint numbers, since the registers had enough bits for it.

After having looked through the code to see what was going on, it became pretty clear that non-sequential endpoint numbers would be a hassle to map between the endpoint registers and buffers. I'm fine with the implementation tradeoff, as it's not a big deal to just use endpoint numbers 1-7.

I'd mostly like to save future users from going down the rabbit hole of adding a handler for an endpoint number > 7 and causing their device to enumerate, but act as if it's unconfigured.

Since there doesn't appear to be any documentation for the individual USB driver backends, I think that adding a note to usbd_ep_setup would be the next most logical place.

@karlp
Copy link
Member

karlp commented Jun 5, 2019

Better late than never, and I know it's just documentation, but thanks for filing this!

BOJIT pushed a commit to BOJIT/PlatformIO-libopencm3 that referenced this issue Jan 30, 2021
The internal stack has a hard internal limit of 8, which is as many as
all supported devices support, but not as flexible as the arbitrary
addressing that USB actually allows.

At _least_ document this.

Fixes: libopencm3#666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants