STMicroelectronics / STM32CubeF4

STM32Cube MCU Full Package for the STM32F4 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
824 stars 408 forks source link

prevent memory corruption #148

Open maggu2810 opened 1 year ago

maggu2810 commented 1 year ago

An USB device that provides more endpoints then the defined maximum supported ones of the code base this will lead to a memory corruption. The code does not check if it runs over the maximum defined endpoints and write to the memory behind of it. For the number of interfaces the check if the maximum is already reached is present, for the endpoints it is missing.

RJMSTM commented 1 year ago

Hello @maggu2810,

Thank you for this fix proposal, however there is another more global proposal suggested via this pull request stm32_mw_usb_host#4

A fix will be implemented and made available in the frame of a future release. Thank you again for your proposal and thank you for your comprehension.

With regards,

RJMSTM commented 1 year ago

ST Internal Reference: 123858

maggu2810 commented 1 year ago

Hm, but the fix your linked results into a different behaviour. Or is my reading not correct?

The current code checks if all endpoints of an interface has been processed and if not all could be processed it refuses to use the device.

        /* Check if the required endpoint(s) data are parsed */
        if (ep_ix < pif->bNumEndpoints)
        {
          return USBH_NOT_SUPPORTED;
        }

The fix you linked will just ignore all endpoints after USBH_MAX_NUM_ENDPOINTS and could lead into another debugging session. Why is the device "accepted" but not working as expected?

And as another point: Your code already contains the check that all endpoints of the interface are used. So, let's assume this has been the desired behaviour. Now you want to drop that check? The current code base also contains the similar check for the number of interfaces - so do you plan to also change this code to magically ignore all interfaces that exceed the limits? I assume the handling of received interfaces and its max define and the handling of received endpoints and its max define should still be handled the same way. Handle interfaces by checking the max in the loop and handle endpoints by changing the max number in the struct seems inconsistent.