walkaround for buffer overflowed by pio_usb_ll_encode_tx_data()

[hathach]

I have been trying to troubleshoot issue for 0.6.0 with -Os. TBH, it is rabbit hole for me since I am not familliar with pio at all.

• https://github.com/adafruit/Adafruit_TinyUSB_Arduino/issues/438
• https://github.com/sekigon-gonnoc/Pico-PIO-USB/issues/122

Luckily I think I possibly found the root cause. Which is an buffer oveflow caused by pio_usb_ll_encode_tx_data() (introduced by https://github.com/sekigon-gonnoc/Pico-PIO-USB/commit/7ce6e4a5caf43275853ab22f213ad094825735be). The function will overflow 1 byte for pre-encoded PID (ack/nak/stall/pre). This can be easily tested with pure C program compiled and run on x86 PC with following main.c

#include <stdio.h>
#include <stdint.h>
#include <string.h>

enum {
  PIO_USB_TX_ENCODED_DATA_SE0 = 0,
  PIO_USB_TX_ENCODED_DATA_K = 1,
  PIO_USB_TX_ENCODED_DATA_COMP = 2,
  PIO_USB_TX_ENCODED_DATA_J = 3,
};

enum {
  USB_SYNC = 0x80,
  USB_PID_OUT = 0xe1,
  USB_PID_IN = 0x69,
  USB_PID_SOF = 0xa5,
  USB_PID_SETUP = 0x2d,
  USB_PID_DATA0 = 0xc3,
  USB_PID_DATA1 = 0x4b,
  USB_PID_ACK = 0xd2,
  USB_PID_NAK = 0x5a,
  USB_PID_STALL = 0x1e,
  USB_PID_PRE = 0x3c,
  USB_CRC16_PLACE = 0,
};

static uint8_t ack_encoded[8];
static uint8_t nak_encoded[8];
static uint8_t stall_encoded[8];
static uint8_t pre_encoded[8];

uint8_t pio_usb_ll_encode_tx_data(
  uint8_t const* buffer, uint8_t buffer_len, uint8_t* encoded_data) {
  uint16_t bit_idx = 0;
  int current_state = 1;
  int bit_stuffing = 6;
  for (int idx = 0; idx < buffer_len; idx++) {
    uint8_t byte = buffer[idx];
    for (int b = 0; b < 8; b++) {
      uint8_t byte_idx = bit_idx >> 2;
      encoded_data[byte_idx] <<= 2;
      if (byte & (1 << b)) {
        if (current_state) {
          encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_K;
        } else {
          encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_J;
        }
        bit_stuffing--;
      } else {
        if (current_state) {
          encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_J;
          current_state = 0;
        } else {
          encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_K;
          current_state = 1;
        }
        bit_stuffing = 6;
      }

      bit_idx++;

      if (bit_stuffing == 0) {
        byte_idx = bit_idx >> 2;
        encoded_data[byte_idx] <<= 2;

        if (current_state) {
          encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_J;
          current_state = 0;
        } else {
          encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_K;
          current_state = 1;
        }
        bit_stuffing = 6;
        bit_idx++;
      }
    }
  }

  uint8_t byte_idx = bit_idx >> 2;
  encoded_data[byte_idx] <<= 2;
  encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_SE0;
  bit_idx++;

  byte_idx = bit_idx >> 2;
  encoded_data[byte_idx] <<= 2;
  encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_COMP;
  bit_idx++;

  do {
    byte_idx = bit_idx >> 2;
    encoded_data[byte_idx] <<= 2;
    encoded_data[byte_idx] |= PIO_USB_TX_ENCODED_DATA_K;
    bit_idx++;
  } while (bit_idx & 0x07);

  byte_idx = bit_idx >> 2;
  return byte_idx;
}

void print_encoded(uint8_t* encoded) {
  for (int i = 0; i < 8; i++) {
    printf("%02x ", encoded[i]);
  }
  printf("\n");
}

int main(void) {
  memset(ack_encoded, 0xaa, 8);
  memset(nak_encoded, 0xaa, 8);
  memset(stall_encoded, 0xaa, 8);
  memset(pre_encoded, 0xaa, 8);

  // pre-encode handshake packets
  uint8_t raw_packet[] = { USB_SYNC, USB_PID_ACK };
  pio_usb_ll_encode_tx_data(raw_packet, 2, ack_encoded);
  raw_packet[1] = USB_PID_NAK;
  pio_usb_ll_encode_tx_data(raw_packet, 2, nak_encoded);
  raw_packet[1] = USB_PID_STALL;
  pio_usb_ll_encode_tx_data(raw_packet, 2, stall_encoded);
  raw_packet[1] = USB_PID_PRE;
  pio_usb_ll_encode_tx_data(raw_packet, 2, pre_encoded);

  for(int i=0; i<8; i++) {
    printf("%02x ", i);
  }
  printf("\n");
  print_encoded(ack_encoded);
  print_encoded(nak_encoded);
  print_encoded(stall_encoded);
  print_encoded(pre_encoded);

  return 0;
}

The output is

00 01 02 03 04 05 06 07 
dd df 5d 7f 25 55 aa aa 
dd df 5f d7 25 55 aa aa 
dd df 55 77 25 55 aa aa 
dd df 7f f7 25 55 aa aa 

all messages is memset with 0xaa and we can clearly see that [5] is written. I guess the reason that we can get away with non -Os is

• compiler give us 8 bytes for array of 5
• and/or even if they give 5 bytes, the latter call will correct the former one by rewrite the message. As long as there is 1 byte padding after pre_encoded, we are all good.

Although I have no clues how to fix this, increase 1 byte may not ideal, since this function is also called in run time. thus this PR is only an placeholder for discussion. @sekigon-gonnoc definitely know the correct fix.

@ladyada

[sekigon-gonnoc]

Thank you for the information. I created a new pull request #132 based on this pull request.

[hathach]

perfect, this pr has done its job.