usb: endpoint buffer leak upon SET_CONFIGURATION, SET_INTERFACE

[jmgao]

Describe the bug
Some platforms don't use a real allocator to allocate endpoint packet buffers, and just bump a pointer into a buffer (stm32, sam0), which results in the buffer getting leaked every time an endpoint is configured as a result of SET_CONFIGURATION or SET_INTERFACE, which quickly leads to configuration failing. It looks like sam0 can be switched over to using a mempool pretty trivially, but stm32 has a dedicated region of memory for USB packets, which makes things harder. It seems to me that the simplest solution would be to add something along the lines of usb_dc_reconfigure to let the driver know it can throw away all of the endpoint buffers.

To Reproduce
Steps to reproduce the behavior:
Build any USB sample, pip3 install pyusb and then run this python script:

import usb.core
import usb.util
dev = usb.core.find(idVendor=0x2FE3)
dev.detach_kernel_driver(0)
while True:
    dev.set_configuration()

and you'll see logs that are something along the lines of the following (along with a broken USB device):

[00:00:00.374,000] <inf> hid: USB_DC_RESET
[00:00:00.476,000] <inf> hid: USB_DC_CONFIGURED
[00:00:03.344,000] <inf> hid: USB_DC_CONFIGURED
[00:00:03.345,000] <inf> hid: USB_DC_CONFIGURED
[00:00:03.345,000] <err> usb_device: Failed to configure endpoint 0x81
[00:00:03.345,000] <err> usb_device: Failed to configure endpoint 0x01
[00:00:03.345,000] <inf> hid: USB_DC_CONFIGURED

[jfischer-no]

It looks like sam0 can be switched over to using a mempool pretty trivially,but stm32 has a dedicated region of memory for USB packets, which makes things harder. It seems to me that the simplest solution would be to add something along the lines of usb_dc_reconfigure to let the driver know it can throw away all of the endpoint buffers.

No need for usb_dc_reconfigure as we have usb_dc_ep_disable(). Independent of the issues in the drivers, usb_set_configuration() could rather also call reset_endpoint() as just set_endpoint(). But as I can remember, we tolerate this because the stack supports only one configuration. It is also up to the driver to check if a endpoint is already configured and not allocate the buffer if not necessary. usb_dc_stm32.c does not track the status of the endpoints, but that could probably done by using ep_mps variable or a new bool enabled in usb_dc_stm32_ep_state.

[jmgao]

But as I can remember, we tolerate this because the stack supports only one configuration.

That's true of configuration right now, but interface alt setting seems to be supported today (and support for multiple configurations is something that probably needs be implemented at some point?)

I switched my stm32 patch over to tracking the size of the buffer allocated and leaking/reallocating if the mps increases, which probably covers the common case?

[eobalski]

It seems to me that the simplest solution would be to add something along the lines of usb_dc_reconfigure to let the driver know it can throw away all of the endpoint buffers.

I dont think this is good approach as it will make an exception for those drivers which does not track the internal configurations. The case is st driver does not free the buffer which was previously allocated here.

I see 2 solutions (st):

• Check if the endpoint has already configured the space for the buffer here If yes return -EALREADY.

• Free memory in usb_dc_ep_disable and add something like
  reset_endpoints() to usb_set_configuration() as @jfischer-phytec-iot mentioned.

I think the 2nd option is not the greatest as it will impact all drivers with additional reset_eps() call.

But as I can remember, we tolerate this because the stack supports only one configuration.

That's true of configuration right now, but interface alt setting seems to be supported today (and support for multiple configurations is something that probably needs be implemented at some point?)

To make it usefull we would first have to have multiple configuration support in the descriptors and we do not support that (and probably would never have ) so there is no need for that at all.

[eobalski]

nvm, I hadn't noticed You have already posted a pr for this.

[jfischer-no]

and support for multiple configurations is something that probably needs be implemented at some point?

Disagree, that is disproportionate effort compared to the use case.

[erwango]

@jmgao @jfischer-phytec-iot, issue is still open, but PR has been merged.
Can we close it ?

[carlescufi]

@jmgao @jfischer-phytec-iot, issue is still open, but PR has been merged.
Can we close it ?

Yes, let's close it. If there are any unresolved issue this can be reopened.

[jfischer-no]

Reopen as it still an issue on usb_dc_sam0