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

prevent memory corruption #148

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

Conversation

maggu2810
Copy link

@maggu2810 maggu2810 commented Nov 4, 2022

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.

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.

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@RJMSTM RJMSTM added bug Something isn't working usb USB-related (host or device) issue or pull-request mw Middleware-related issue or pull-request. spotted before customer Spotted and fixed internally before being pointed out by users but not published yet internal bug tracker Issue confirmed and reported into a ticket in the internal bug tracking system labels Nov 10, 2022
@RJMSTM
Copy link
Contributor

RJMSTM commented Nov 14, 2022

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
Copy link
Contributor

RJMSTM commented Nov 14, 2022

ST Internal Reference: 123858

@RJMSTM RJMSTM moved this from To do to To release in stm32cube-mcu-fw-dashboard Nov 14, 2022
@RJMSTM RJMSTM moved this from To release to In progress in stm32cube-mcu-fw-dashboard Nov 14, 2022
@maggu2810
Copy link
Author

maggu2810 commented Nov 15, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working internal bug tracker Issue confirmed and reported into a ticket in the internal bug tracking system mw Middleware-related issue or pull-request. spotted before customer Spotted and fixed internally before being pointed out by users but not published yet usb USB-related (host or device) issue or pull-request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants