[bugfix] Restore overflowed memory copy

[bigtreetech]

Arduino_STM32/STM32F1/system/libmaple/include/libmaple/usb_cdcacm.h

Line 155 in a3a5686

} __packed usb_cdcacm_line_coding;

This structure takes up 7 bytes of RAM, under some memory-aligned compilation rules, it takes up 8bytes
This structural variable is passed into the
PMAToUserBufferCopy function as a
u8 * pbUsrBufparameter in here

Arduino_STM32/STM32F1/cores/maple/libmaple/usb/usb_lib/usb_mem.c

Line 60 in a3a5686

void PMAToUserBufferCopy(u8 *pbUsrBuf, u16 wPMABufAddr, u16 wNBytes)

If the length of the pbUsrBuf parameter is odd, it will tamper with the last byte of the address.
If the compiler is single-byte aligned, the structure line_coding takes up 7bytes, At this point, the value of the variable that follows line_coding will be tampered by PMAToUserBufferCopy.
for example：

usb_cdcacm_line_coding line_coding; //@ram address 0x20000640
uint8_t test = 0; //@ram address 0x20000647

The normal value of the test variable is 0. When the

PMAToUserBufferCopy((u8 *)&line_coding, xxx, 7)

function has been executed
The value of the test variable will be tampered, not equal 0.

thie PR restored tampered data.

[stevstrong]

I think the issue is line 68-69:

    *(u16*)pbUsrBuf++ = *pdwVal++;
    pbUsrBuf++;

The 4 bytes pointed by the pbUsrBuf will be overwritten, pdwVal is a 32 bit pointer.
It was probably meant to optimize the speed but this issue was neglected.

I think the best solution would be to add a dummy byte to the usb_cdcacm_line_coding struct (between lines 154-155) so that the memory allocation will always reserve 8 bytes, this way the speed advantage is kept.
Something like this:

    uint8 bDataBits;            /* Data bits: 5, 6, 7, 8, or 16 */
    uint8 dummy;            // reserved, see issue #661 
} __packed usb_cdcacm_line_coding;

[stevstrong]

Hm, having a closer look at the function PMAToUserBufferCopy, it seems to me that it will copy twice as much data as needed.

void PMAToUserBufferCopy(u8 *pbUsrBuf, u16 wPMABufAddr, u16 wNBytes)
{
  u32 n = (wNBytes + 1) >> 1;/* /2*/   //------------------  n = wNBytes / 2
  u32 i;
  u32 *pdwVal;
  pdwVal = (u32 *)(wPMABufAddr * 2 + PMAAddr);
  for (i = n; i != 0; i--)            //------------------- this will loop n times
  {
    *(u16*)pbUsrBuf++ = *pdwVal++;    //------------------- this will copy 4 bytes
    pbUsrBuf++;
  }
}

Result: n x 4 bytes will be copied.
If wNBytes = 7, then 4*4=16 bytes will be copied.
This is 2 times more than expected.

Or do I miss something?

[stevstrong]

I was missing that the packet buffer memory, as well as all USB registers, are aligned to 32-bit word boundaries although they are 16-bit wide only, see RM0008, chapter 23.5.
So only the 2 lower bytes of a 32-bit segment are used.
Taking this into account, a correct version of the copy routine should look like:

void PMAToUserBufferCopy(u8 *pbUsrBuf, u16 wPMABufAddr, u16 wNBytes)
{
  u32 *pdwVal = (u32 *)(wPMABufAddr * 2 + PMAAddr);
  while (wNBytes--)
  {
    uint32 temp = *pdwVal++;
    *pbUsrBuf++ = (uint8) temp;
    if (wNBytes--)
    {
      temp >>= 8;
      *pbUsrBuf++ = (uint8) temp;
    }
  }
}

Can you please check if this solves the issue?

[bigtreetech]

I think the issue is line 68-69:

    *(u16*)pbUsrBuf++ = *pdwVal++;
    pbUsrBuf++;

The 4 bytes pointed by the pbUsrBuf will be overwritten, pdwVal is a 32 bit pointer.
It was probably meant to optimize the speed but this issue was neglected.

I think the best solution would be to add a dummy byte to the usb_cdcacm_line_coding struct (between lines 154-155) so that the memory allocation will always reserve 8 bytes, this way the speed advantage is kept.
Something like this:

    uint8 bDataBits;            /* Data bits: 5, 6, 7, 8, or 16 */
    uint8 dummy;            // reserved, see issue #661 
} __packed usb_cdcacm_line_coding;

yeah, dummy reserved byte can solve my problem, But in fact, the parameters wNBytes of this function

void PMAToUserBufferCopy(u8 *pbUsrBuf, u16 wPMABufAddr, u16 wNBytes)

can only be even number, cann't be odd number

[bigtreetech]

I was missing that the packet buffer memory, as well as all USB registers, are aligned to 32-bit word boundaries although they are 16-bit wide only, see RM0008, chapter 23.5.
So only the 2 lower bytes of a 32-bit segment are used.
Taking this into account, a correct version of the copy routine should look like:

void PMAToUserBufferCopy(u8 *pbUsrBuf, u16 wPMABufAddr, u16 wNBytes)
{
  u32 *pdwVal = (u32 *)(wPMABufAddr * 2 + PMAAddr);
  while (wNBytes--)
  {
    uint32 temp = *pdwVal++;
    *pbUsrBuf++ = (uint8) temp;
    if (wNBytes--)
    {
      temp >>= 8;
      *pbUsrBuf++ = (uint8) temp;
    }
  }
}

Can you please check if this solves the issue?

It looks okay.

[stevstrong]

Yea, it looks okay, but it is not fast.
A faster version could be this:

void PMAToUserBufferCopy(u8 *pbUsrBuf, u16 wPMABufAddr, u16 wNBytes)
{
  u16 * destPtr = (u16*)pbUsrBuf;
  u32 * pdwVal = (u32 *)(wPMABufAddr * 2 + PMAAddr);
  for (u16 i = wNBytes/2; i > 0; i--)
  {
    *destPtr++ = (u16)*pdwVal++;
  }
  if (wNBytes & 0x1) // odd value ?
  {
    *(u8*)destPtr = *(u8*)pdwVal;
  }
}

The difference to the original is that the number of times to copy data in word (16-bit) format will not be rounded up.
This way the last odd byte will be copied separately at the end.

[stevstrong]

I merged my variant as it seems to be a more effective solution than yours.
Tested and it works.
Please test it yourself and let me know if it does not work for you.