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

Buffer overrun with USBH_CDC_Receive #9

Open
Rimpampa opened this issue Oct 4, 2022 · 2 comments
Open

Buffer overrun with USBH_CDC_Receive #9

Rimpampa opened this issue Oct 4, 2022 · 2 comments
Assignees
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. usb USB-related (host or device) issue or pull-request

Comments

@Rimpampa
Copy link

Rimpampa commented Oct 4, 2022

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 ALABSTM added mw Middleware-related issue or pull-request. usb USB-related (host or device) issue or pull-request labels May 15, 2023
@ALABSTM
Copy link
Collaborator

ALABSTM commented Feb 13, 2024

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 ALABSTM added bug Something isn't working internal bug tracker Issue confirmed and reported into a ticket in the internal bug tracking system. labels Feb 13, 2024
@ALABSTM
Copy link
Collaborator

ALABSTM commented Feb 13, 2024

ST Internal Reference: 173367

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. usb USB-related (host or device) issue or pull-request
Projects
Status: In progress
Development

No branches or pull requests

3 participants