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

Buffer overrun with USBH_CDC_Receive #9

Open Rimpampa opened 2 years ago

Rimpampa commented 2 years ago

Introduction

I'm working with a F4 family ST MCU and I'm using this middleware to communicate with a CDC USB device.

The code involved is somewhat like this:

extern USBH_HandleTypeDef hUsbHostFS;

bool rx_end = true;

void recv(uint8_t data[], uint32_t size) {
  USBH_CDC_Receive(&hUsbHostFS, data, size);
  rx_end = false;
}

void USBH_CDC_ReceiveCallback(USBH_HandleTypeDef *phost) {
  rx_end = true;
}

void recv_all(uint8_t data[], uint32_t size) {
    uint32_t at = 0;
    while(at < size) {
        recv(data + at, size - at);
        while(!rx_end);
        at += USBH_CDC_GetLastReceivedDataSize(&hUsbHostFS);
    }
}

(for the scope of this issue I've excluded many important parts like synchronization of the global state access and error handling)

The problem

That code works fine most of the times but sometimes it causes a buffer overrun when the data buffer is too small (or the at index is too close to the end of data).

This becomes obvious when I call recv_all(buffer, 0) and see that data is still being received.

Analysis

I want to state right away that I'm beginner and this could be all wrong as I haven't had the time to dig deeper into this problem.

The main cause of this problem I think resides in the implementation of CDC_ProcessReception file.

We can see that USBH_CDC_Receive uses the parameter its given to set the pRxData and RxDataLength fields:

CDC_Handle->pRxData = pbuff;
CDC_Handle->RxDataLength = length;

In the CDC_ProcessReception, though, we can see this:

USBH_BulkReceiveData(
    phost,
    CDC_Handle->pRxData,
    CDC_Handle->DataItf.InEpSize,
    CDC_Handle->DataItf.InPipe
);

The problem with that is that it's not even considering the RxDataLength field, which in turn means that if you call USBH_CDC_Receive with a buffer that is smaller than CDC_Handle->DataItf.InEpSize undefined behaviour will be generated.

One important note is that CDC_ProcessTransmission handles this problem:

if(CDC_Handle->TxDataLength > CDC_Handle->DataItf.OutEpSize) {
    USBH_BulkSendData(
        phost,
        CDC_Handle->pTxData,
        CDC_Handle->DataItf.OutEpSize,
        CDC_Handle->DataItf.OutPipe,
        1U
    );
} else {
    USBH_BulkSendData(
        phost,
        CDC_Handle->pTxData,
        (uint16_t)CDC_Handle->TxDataLength,
        CDC_Handle->DataItf.OutPipe,
        1U
    );
}

Suggestion

If this is expected I feel like it should be explained better in the documentation as I couldn't find any warning about this, but if, instead, this is a problem then I would like to know if a patch could be available in the near future.

ALABSTM commented 9 months ago

Hi @Rimpampa,

Please excuse this delayed reply. Your point seems very relevant. It has been forwarded to our development teams. I will keep you informed.

With regards,

ALABSTM commented 9 months ago

ST Internal Reference: 173367