Stack corruption in MSC_File_Operations()

[stsymbaliuk]

There are a lot of same files file_operations.c in different projects and different MCU series Cube packages. Now I use F7 and found error in MSC_File_Operations() all other files have the same problem.

Stack corruption caused by f_read() which expects 32 bits variable pointer but bytesread is 16 bits. When f_read() called it overrides 2 bytes of bytesread and extra 2 bytes of stack were other variables located.

To fix it all uint16_t bytesread; must be replaced with UINT bytesread; Also pointer to void cast should be removed: Replace f_read(&MyFile, rtext, sizeof(rtext), (void *)&bytesread) With: f_read(&MyFile, rtext, sizeof(rtext), &bytesread)

Where I can report this issue to fix it in all files at a time?

file_operations.c

void MSC_File_Operations(void)
{
  uint16_t bytesread;
  ...
  res = f_read(&MyFile, rtext, sizeof(rtext), (void *)&bytesread);
  ...
}

ff.h

FRESULT f_read (
    FIL* fp,    /* Pointer to the file object */
    void* buff, /* Pointer to data buffer */
    UINT btr,   /* Number of bytes to read */
    UINT* br    /* Pointer to number of bytes read */
)

integer.h

typedef unsigned int    UINT;

Also I've checked sizeof(unsigned int)

printf("sizeof(unsigned int): %zu\n", sizeof(unsigned int));

Result is:

sizeof(unsigned int): 4

[ALABSTM]

Hi @stsymbaliuk,

Thank you for this report, clear and concise. Thank you also for the fix proposal.

May I ask you whether you experienced a crash or a stack corruption at run-time or is this an aspect you identified through code review? In the first case, would you mind detailing the procedure to reproduce the issue?

Thank you in advance for your answers.

With regards,

[stsymbaliuk]

Hi @ALABSTM

I see this problem in runtime. It is hard to debug, as corrupted variable can appear in function that call MSC_File_Operations(). Before call MSC_File_Operations() locals variable of caller function was saved to stack. As MSC_File_Operations() don't use stack for other variables except bytesread it brakes that caller saved variable.

To reproduce I've created a local array of uint16_t bytesread_test[3]. Local variables created in stack. And I use array as array elements are unaligned (they located in continues memory region without gaps). So bytesread_test[1] used as f_read() parameter (same way bytesread was used in original code). After f_read() call not only bytesread_test[1] but bytesread_test[2] was changed. So from this you can see that problem exists. Or you can debug original code with code stepping by disassembled code and track stack state before and after MSC_File_Operations() as I did to find the reason of my problem.

Issue fix described in my first message.

To reproduce problem I've used example project for STM32F769I-Discovery: \STM32Cube\Repository\STM32Cube_FW_F7_V1.16.0\Projects\STM32F769I-Discovery\Applications\USB_Host\MSC_Standalone

Only MSC_File_Operations() changed to reproduce the issue.

Code that demonstrates the problem. To see fixed code work change USE_CODE_WITH_PROBLEM to 0.

void MSC_File_Operations(void)
{
#define USE_CODE_WITH_PROBLEM 1

#if USE_CODE_WITH_PROBLEM
#define INIT_VALUE UINT16_MAX
  uint16_t bytesread_test[3] = {INIT_VALUE, INIT_VALUE, INIT_VALUE};
#else
#define INIT_VALUE UINT32_MAX
  UINT bytesread_test[3] = {INIT_VALUE, INIT_VALUE, INIT_VALUE};
#endif

  LCD_UsrLog("INFO : FatFs Initialized \n");

  if (f_open(&MyFile, "0:USBHost.txt", FA_CREATE_ALWAYS | FA_WRITE) != FR_OK)
  {
    LCD_ErrLog("Cannot Open 'USBHost.txt' file \n");
  }
  else
  {
    LCD_UsrLog("INFO : 'USBHost.txt' opened for write  \n");
    res = f_write(&MyFile, wtext, sizeof(wtext), (void *)&bytesWritten);
    f_close(&MyFile);

    if ((bytesWritten == 0) || (res != FR_OK))  /* EOF or Error */
    {
      LCD_ErrLog("Cannot Write on the  'USBHost.txt' file \n");
    }
    else
    {
      if (f_open(&MyFile, "0:USBHost.txt", FA_READ) != FR_OK)
      {
        LCD_ErrLog("Cannot Open 'USBHost.txt' file for read.\n");
      }
      else
      {
        LCD_UsrLog("INFO : Text written on the 'USBHost.txt' file \n");

#if USE_CODE_WITH_PROBLEM
        res = f_read(&MyFile, rtext, sizeof(rtext), (void *)&bytesread_test[1]);
#else
        res = f_read(&MyFile, rtext, sizeof(rtext), &bytesread_test[1]);
#endif

        LCD_DbgLog("bytesread_test[0]: %04x\n", bytesread_test[0]);
        LCD_DbgLog("bytesread_test[1]: %04x\n", bytesread_test[1]);
        LCD_DbgLog("bytesread_test[2]: %04x\n", bytesread_test[2]);

        if (bytesread_test[2] != INIT_VALUE)
        {
          LCD_DbgLog("Error - Variable in stack changed, bytesread_test[2]: %04x\n", bytesread_test[2]);
        }
        else
        {
          LCD_DbgLog("Ok - Variable in stack unchanged, bytesread_test[2]: %04x\n", bytesread_test[2]);
        }

        if ((bytesread_test[1] == 0) || (res != FR_OK)) /* EOF or Error */
        {
          LCD_ErrLog("Cannot Read from the  'USBHost.txt' file \n");
        }
        else
        {
          LCD_UsrLog("Read Text : \n");
          LCD_DbgLog((char *)rtext);
          LCD_DbgLog("\n");
        }
        f_close(&MyFile);
      }
      /* Compare read data with the expected data */
      if ((bytesread_test[1] == bytesWritten))
      {
        LCD_UsrLog("INFO : FatFs data compare SUCCES");
        LCD_UsrLog("\n");
      }
      else
      {
        LCD_ErrLog("FatFs data compare ERROR");
        LCD_ErrLog("\n");
      }
    }
  }
}

[ALABSTM]

Hi @stsymbaliuk,

I hope you are fine. Allow me first to thank you for all these details and explanations. Your report has been forwarded to our development team. They confirmed the issue. I will get back to you as soon as I have more details.

With regards,

[ALABSTM]

ST Internal Reference: 121879

[ALABSTM]

Fixed in commit 043626d52e34ee8146588bae597513398683e329

[ALABSTM]

Hi @stsymbaliuk,

Back to you about this issue you raised a long time ago. The fix has been finally published. Thank you again for having reported. Spotting the issue was not obvious.

With regards,