USB CDC Receive Buffer overflow

theadib commented 2 years ago

Caution The Issues are strictly limited for the reporting of problem encountered with the software provided in this project. For any other problem related to the STM32 product, the performance, the hardware characteristics and boards, the tools the environment in general, please post a topic in the ST Community/STM32 MCUs forum.

Describe the set-up

The board (either ST RPN reference or your custom board).
IDE or at least the compiler and its version.
using STMCubeMX 6.5 and STM32F4 lib 1.27

Describe the bug A clear and concise description of what the bug is.

when in the IDE USB_device CDC Setting a rather small USB CDC RX Buffer Size is selected. in my case 8Byte
then in the receive function there is an buffer overflow affecting the application.

in fact in the file usbd_cdc_if.c there is this function call line 163: USBD_CDC_SetRxBuffer(&hUsbDeviceFS, UserRxBufferFS); this sets the buffer pointer without telling the size of the buffer.

That function copies the pointer without telling the size of the buffer

uint8_t USBD_CDC_SetRxBuffer(USBD_HandleTypeDef *pdev, uint8_t *pbuff)
{
  USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef *)pdev->pClassDataCmsit[pdev->classId];
  if (hcdc == NULL)
  {
    return (uint8_t)USBD_FAIL;
  }
  hcdc->RxBuffer = pbuff;
  return (uint8_t)USBD_OK;
}

then in the file usbd_cdc.c in the function USBD_CDC_ReceivePacket() this buffer is used with an expected buffer size:

uint8_t USBD_CDC_ReceivePacket(USBD_HandleTypeDef *pdev)
{
  USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef *)pdev->pClassDataCmsit[pdev->classId];

#ifdef USE_USBD_COMPOSITE
  /* Get the Endpoints addresses allocated for this class instance */
  CDCOutEpAdd = USBD_CoreGetEPAdd(pdev, USBD_EP_OUT, USBD_EP_TYPE_BULK);
#endif /* USE_USBD_COMPOSITE */

  if (pdev->pClassDataCmsit[pdev->classId] == NULL)
  {
    return (uint8_t)USBD_FAIL;
  }

  if (pdev->dev_speed == USBD_SPEED_HIGH)
  {
    /* Prepare Out endpoint to receive next packet */
>    (void)USBD_LL_PrepareReceive(pdev, CDCOutEpAdd, hcdc->RxBuffer,
>                                 CDC_DATA_HS_OUT_PACKET_SIZE);
  }
  else
  {
    /* Prepare Out endpoint to receive next packet */
>    (void)USBD_LL_PrepareReceive(pdev, CDCOutEpAdd, hcdc->RxBuffer,
>                                 CDC_DATA_FS_OUT_PACKET_SIZE);
  }

  return (uint8_t)USBD_OK;
}

the expected buffer size is dependent on the interface.

So the minimal buffersize must be CDC_DATA_FS_OUT_PACKET_SIZE or CDC_DATA_HS_OUT_PACKET_SIZE.

To avoid this kind of bugs always give the buffersize together with the buffer pointer via the API call.

How To Reproduce

Indicate the global behavior of your application project. select 8Bytes as USB CDC Rx Buffer Size
The modules that you suspect to be the cause of the problem (Driver, BSP, MW ...). USB CDC
The use case that generates the problem.
How we can reproduce the problem.

Additional context If you have a first analysis or patch correction, thank you to share your proposal.

Screenshots If applicable, add screenshots to help explain your problem.

ALABSTM commented 2 years ago

Hi @theadib,

Thank you for this detailed report. You are absolutely right. The UserRxBufferFS buffer size should be equal to CDC_DATA_FS_OUT_PACKET_SIZE at least and a multiple of it in general. The same applies for an eventual UserTxBufferFS buffer.

To summarize, this is an issue in Cube MX tool. This will be forwarded to our development teams to have it fixed in a future Cube MX release. Below are more details.

The CDC_DATA_FS_OUT_PACKET_SIZE constant, along with others alike, is defined in the usbd_cdc.h library file, as shown below.

https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Inc/usbd_cdc.h#L67-L71

https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Inc/usbd_cdc.h#L62-L63

Now, at application level, buffer sizes are not defined based on the above mentioned constants, rather based on other ones shown below and whose definitions are generated by Cube MX.

https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Projects/STM32446E_EVAL/Applications/USB_Device/CDC_Standalone/Src/usbd_cdc_interface.c#L23-L24

The problem is that currently these constants could be assigned any value by the user. Instead, only buffer sizes multiple of CDC_DATA_FS_OUT_PACKET_SIZE or CDC_DATA_HS_OUT_PACKET_SIZE should be allowed.

With regards,

ALABSTM commented 2 years ago

ST Internal Reference: 126941

ALABSTM commented 2 years ago

Hi @theadib,

A ticket has been logged internally to track this issue.

As the issue is not related to the software published within this repository, rather to the STM32 ecosystem, no fix is expected to be published here. Hence, please allow me to close it. Thank you for your comprehension and thank you again for your report.

With regards,

theadib commented 2 years ago

Hello Ali,

After updating to the stm33f4 libs 1.27 in one Project we suffer the issue that ethernet does not work anymore. ST has changed the way of driver integration. Once I identify the reason I will file a ticket regardingly.

However what I see is that it is possible to change the API. And I think this is more important to change the API if the original API has broken design. This setTXBuffer setRxBuffer API in usb CDC IS broken design. And we have issued tickets. Now this ticket here only could exists because of this broken design.

Please fix this broken design rather than implement this crude workarounds around the places.

Please add the buffer length as additional parameter in the API to the setTxBuffer setRxBuffer. That is quite simple.

Thanks, Adib.

Ali LABBENE @.***> schrieb am Mo., 25. Apr. 2022, 16:57:

Hi @theadib https://github.com/theadib,

Thank you for this detailed report. You are absolutely right. The UserRxBufferFS buffer size should be equal to CDC_DATA_FS_OUT_PACKET_SIZE at least and a multiple of it in general. The same applies for an eventual UserTxBufferFS buffer.

To summarize, this is an issue in Cube MX tool. This will be forwarded to our development teams to have it fixed in a future Cube MX release. Below are more details.

The CDC_DATA_FS_OUT_PACKET_SIZE constant, along with others alike, is defined in the usbd_cdc.h https://github.com/STMicroelectronics/STM32CubeF4/blob/master/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Inc/usbd_cdc.h library file, as shown below.

https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Inc/usbd_cdc.h#L67-L71

https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Inc/usbd_cdc.h#L62-L63

Now, at application level, buffer sizes are not defined based on the above mentioned constants, rather based on other ones shown below and whose definitions are generated by Cube MX.

https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Projects/STM32446E_EVAL/Applications/USB_Device/CDC_Standalone/Src/usbd_cdc_interface.c#L23-L24

The problem is that currently these constants could be assigned any value by the user. Instead, only buffer sizes multiple of CDC_DATA_FS_OUT_PACKET_SIZE or CDC_DATA_HS_OUT_PACKET_SIZE should be allowed.

With regards,

— Reply to this email directly, view it on GitHub https://github.com/STMicroelectronics/STM32CubeF4/issues/117#issuecomment-1108684731, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFMAKRXOESWTHZTQA3UHZWLVG2XD5ANCNFSM5RDAAPSQ . You are receiving this because you were mentioned.Message ID: @.***>

STMicroelectronics / STM32CubeF4

USB CDC Receive Buffer overflow #117

Thanks, Adib.