USB transfer in serial_usb.c overflows linecoding buffer

[lilvinz]

Hi Giovanni,

in serial_usb.c there is static cdc_linecoding_t linecoding which is declared as follows:

Code: Select all

typedef struct {
  uint8_t                       dwDTERate[4];
  uint8_t                       bCharFormat;
  uint8_t                       bParityType;
  uint8_t                       bDataBits;
} cdc_linecoding_t;

I am compiling using -ftlo -Os and the .map file around that looks like this:

Code: Select all

 .data.msd_string3
                0x20001290       0x32 /tmp/ccdWq3gw.ltrans15.ltrans.o
 .data.linecoding
                0x200012c2        0x7 /tmp/ccdWq3gw.ltrans15.ltrans.o
 .data.divemode_active_screen.lto_priv.225
                0x200012c9        0x1 /tmp/ccdWq3gw.ltrans16.ltrans.o
                0x200012c9                divemode_active_screen.lto_priv.225
 *fill*         0x200012ca        0x2 
 .data.mix_ctx_mutex
                0x200012cc       0x10 /tmp/ccdWq3gw.ltrans19.ltrans.o

So the linecoding struct is of size 7. Using wireshark i can see the usb transferring these 7 bytes and using a debugger i can see that linecoding in memory matches the transferred data.
The issue is that the following variable of size 1 is being overwritten.

Further investigation lead me to otg_fifo_read_to_buffer() in usb_lld.c where i was able to catch the overwrite:

Code: Select all

__attribute__((optimize(0)))
static void otg_fifo_read_to_buffer(volatile uint32_t *fifop,
                                    uint8_t *buf,
                                    size_t n,
                                    size_t max) {

  if (buf == (uint8_t*)0x200012c2)
    asm("nop");

  n = (n + 3) / 4;
  max = (max + 3) / 4;
  while (n) {
    uint32_t w = *fifop;
    if (max) {
      /* Note, this line relies on the Cortex-M3/M4 ability to perform
         unaligned word accesses and on the LSB-first memory organization.*/
      *((PACKED_VAR uint32_t *)buf) = w;
      buf += 4;
      max--;
    }
    n--;
  }
}

When hitting this, buf points to the linecoding struct and n as well as max are 7.

As i read this code, it writes in chunks of 4 bytes which basically explains the overflow.
Is this intended behaviour? If yes, there should be debug checks preventing usb operations with (n % 4) != 0 and usb_serial.c would have to be fixed.
The USBv1 driver seems to be affected similarly.

Thanks and cheers

Vinz

gcc version 4.9.3 20150303 (release) [ARM/embedded-4_9-branch revision 221220] (GNU Tools for ARM Embedded Processors)
ChibiOS 2.6.8
STM32F4 (OTGv1)

[Giovanni]

Hi,

I have to think about it, adding checks would slow down operations while copying buffers.

Giovanni

[lilvinz]

Thanks for the quick reply.

The following code fixes the issue for me although i have no idea about impact on performance:

Code: Select all

static void otg_fifo_read_to_buffer(volatile uint32_t *fifop,
                                    uint8_t *buf,
                                    size_t n,
                                    size_t max) {
  n = (n + 3) / 4;
  while (n) {
    uint32_t w = *fifop;
    if (max >= 4) {
      /* Note, this line relies on the Cortex-M3/M4 ability to perform
         unaligned word accesses and on the LSB-first memory organization.*/
      *((PACKED_VAR uint32_t *)buf) = w;
      buf += 4;
      max -= 4;
    }
    else
    {
      memcpy(buf, &w, max);
    }
    n--;
  }
}

Cheers

Vinz

[lilvinz]

Ping Giovanni,

I consider this a serious bug. Would you mind getting back to it?

Cheers

Vinz

[Giovanni]

I am undecided. Probably the best way is to document the requirement on buffers.

You workaround:
- Enforces a check for each cycle.
- Forces variable w to be allocated in memory instead of a register because the address is taken.
- Makes a final function call with another loop inside.

I don't know exactly how much but it looks like it has a significant impact.

On the other hand variables are aligned to 4 bytes, making a 7 bytes buffer is not going to save memory compared to one of 8 bytes unless the variable that follows is a char (chars are not aligned).

Giovanni

[Giovanni]

Hi,

I am doing another pass on the USB code, I committed a solution for this issue, do you mind help me with testing? thanks.

viewtopic.php?f=3&t=2805&p=22114#p22114

Giovanni