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

CDC + DWC2: OUT endpoint buffer overflow and lost bytes for certain buffer sizes #2832

Open
GGsP839X opened this issue Oct 7, 2024 · 1 comment
Labels

Comments

@GGsP839X
Copy link

GGsP839X commented Oct 7, 2024

Board

STM32U5 custom

Firmware

STM32U5 (dwc2) – FS USB – 1 CDC class – minimal implementation, unmodified from examples/device/cdc_dual_ports besides buffer sizes (and using only one CDC class).

Two bugs occur:

  1. If CFG_TUD_CDC_RX_BUFSIZE is less than CFG_TUD_CDC_EP_BUFSIZE, no tud_cdc_rx_cb RX callback happens and the stack stalls (see CDC epout_buf and epin_buf sizes should be different #1924 (comment) for the same bug).
  2. If the OUT endpoint buffer size is less than twice the USB packet size and less than the amount of bytes sent by the host, the OUT endpoint buffer and subsequently the RX FIFO will miss bytes (for example: buffer size 70, transmitted bytes 80 -> 10 bytes lost). Even worse, those lost bytes get still written to memory beyond the linear buffer (memory overflow corruption).

Solution:

Either the buffer overflow bug detailed below must be solved, or atleast the examples/documentation should be modified:

  1. CFG_TUD_CDC_RX_BUFSIZE has to be >= CFG_TUD_CDC_EP_BUFSIZE.
  2. CFG_TUD_CDC_EP_BUFSIZE has to be >= 128 for FS resp. >= 1024 for HS.

How to reproduce:

  1. Use dwc2, FS USB and (atleast) one CDC class.
  2. Configure buffer sizes in tusb_config.h:
#define CFG_TUD_CDC_RX_BUFSIZE   1024
#define CFG_TUD_CDC_TX_BUFSIZE   1024

#define CFG_TUD_CDC_EP_BUFSIZE     70
  1. Transmit more than 70 bytes (e. g. 80) from host to device.

Debug Log:

Defintion of OUT endpoint buffer (CFG_TUD_CDC_EP_BUFSIZE=70) in cdc_device.c:
2
2

As the RX FIFO (CFG_TUD_CDC_RX_BUFSIZE=1024) has plenty of space available, the OUT endpoint is prepared for xfer in _prep_out_transaction():
2

The call is forwarded by usbd_edpt_xfer() in usbd.c to dcd_edpt_xfer() in dcd_dwc2.c, still with the correct length (total_bytes = CFG_TUD_CDC_EP_BUFSIZE):
2

If the host transmits for example 80 bytes (b'0123456789012345678901234567890123456789012345678901234567890123456789ABCDEFGHIJ' in this case), those get split into two USB FS packages (64 bytes + 16 bytes). handle_rxflvl_irq() is called twice without return in between by
2
with bcnt=(64, 16), both times directly writing into the linear buffer:
2
No length check against xfer->total_len happens here, the bytes are directly written to memory (each 4 bytes as double words first, the remaining 1-3 bytes following):
2

Memory of xfer->buffer after the first copy (bcnt=64):
2
Memory of xfer->buffer after the second copy (bcnt=16), the 70 bytes long OUT endpoint buffer is overflown by 10 bytes (b'ABCDEFGHIJ' in this case):
2

We can potentially overflow to up to two FS packages (128 bytes) until the xfer->buffer pointer is reset – tested by transmitting 1kB of b'X':
2
The same should happen with HS USB, the overflow should increase to up to 1024 bytes.

@HiFiPhile
Copy link
Collaborator

HiFiPhile commented Oct 7, 2024

Thanks for your detailed explanation.

I agree the documentation should be improved:

  • CFG_TUD_CDC_RX_BUFSIZE >= CFG_TUD_CDC_EP_BUFSIZE should be added as an assert.
  • CFG_TUD_CDC_EP_BUFSIZE has to be multiple of endpoint size (64,128, etc. for FS) otherwise DCD may not working correctly, in case of DWC2 it's mentioned in IP's programming guide but not in STM32's reference manual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants