STMicroelectronics / stm32-mw-usb-host

Provides the USB Host library part of the STM32Cube MCU Component "middleware" for all STM32xx series.
Other
32 stars 15 forks source link

Overrun when interface ep_ix >= USBH_MAX_NUM_ENDPOINTS #5

Closed thornley-permaconn closed 1 year ago

thornley-permaconn commented 2 years ago

Referencing: -

https://github.com/STMicroelectronics/stm32_mw_usb_host/blob/70f090d8ff1714c6eb9a48c3d59f2065cb0a6636/Core/Src/usbh_ctlreq.c#L479

Ep_Desc[ep_ix] will overrun memory if device plugged in with endpoints greater than the preallocated USBH_MAX_NUM_ENDPOINTS array (within the interface). Protection is present for if_ix (USBH_MAX_NUM_INTERFACES) but not ep_ix.

thornley-permaconn commented 2 years ago

Thanks @ALABSTM

ALABSTM commented 2 years ago

Hi @thornley-permaconn,

Thank you for having reported this point. Actually, it looks like @Defonceuse submitted a pull-request with a fix proposal. The point will be forwarded to our development teams. I will get back to you as soon as I have their feedback.

May I ask you whether you noticed the point just by reviewing the code or whether you actually experienced a failure due to this implementation? Thank you in advance for your reply.

With regards,

thornley-permaconn commented 2 years ago

Hi @ALABSTM,

I was just running a proof of concept. It is not uncommon to have a large number of endpoints on cellular devices for example.

No rush on my part, I just wanted to do the right thing by reporting it.

ALABSTM commented 2 years ago

Hi @thornley-permaconn,

Thank you very much for the time taken to report. Your point has been submitted to our development teams, as already said. If confirmed, it will be logged into our internal database to have it fixed. I will keep you updated.

With regards,

ALABSTM commented 2 years ago

ST Internal Reference: 123858

ALABSTM commented 2 years ago

ST Internal Reference: 123858

sburgh01 commented 1 year ago

Hi, I had a similar error on annother line, which is still present in the current release. https://github.com/STMicroelectronics/stm32_mw_usb_host/blob/8b21b5c4a6b8df42ad46b9df63bb87c47bd7b2f5/Core/Src/usbh_core.c#L357

i fixxed it in my project locally by including a check in the while loop in Line 355 https://github.com/STMicroelectronics/stm32_mw_usb_host/blob/8b21b5c4a6b8df42ad46b9df63bb87c47bd7b2f5/Core/Src/usbh_core.c#L355 Replacing it by:

  while ((if_ix < USBH_MAX_NUM_INTERFACES) && (if_ix < pcfg->bNumInterfaces))

You could consider adding this when resolving this.

Best regards

sburgh01 commented 1 year ago

As further Info. This made problemes when switching devices with different number of Endpoints, because the content is never cleared and then the Endpoints are misinterpreted on the next Device.