From cb497eec0b56bdfa81d9951eb96f88a387fbe0c5 Mon Sep 17 00:00:00 2001 From: Jan Pochyla Date: Tue, 28 Mar 2017 12:30:45 +0200 Subject: [PATCH] trezorhal: error checks, fix potential bug in usb_hid_class_data_out --- micropython/trezorhal/usb.c | 4 ++ micropython/trezorhal/usb_hid-impl.h | 83 +++++++++++++++++----------- 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/micropython/trezorhal/usb.c b/micropython/trezorhal/usb.c index eea6696c..71fadef1 100644 --- a/micropython/trezorhal/usb.c +++ b/micropython/trezorhal/usb.c @@ -10,6 +10,10 @@ #define USB_MAX_CONFIG_DESC_SIZE 128 #define USB_MAX_STR_DESC_SIZE 256 +#define USB_EP_DIR_OUT 0x00 +#define USB_EP_DIR_IN 0x80 +#define USB_EP_DIR_MSK 0x80 + extern PCD_HandleTypeDef pcd_fs_handle; static USBD_HandleTypeDef usb_dev_handle; diff --git a/micropython/trezorhal/usb_hid-impl.h b/micropython/trezorhal/usb_hid-impl.h index d1bfc7b6..fb1348f2 100644 --- a/micropython/trezorhal/usb_hid-impl.h +++ b/micropython/trezorhal/usb_hid-impl.h @@ -9,19 +9,20 @@ /* usb_hid_add adds and configures new USB HID interface according to * configuration options passed in `info`. */ int usb_hid_add(const usb_hid_info_t *info) { - usb_hid_descriptor_block_t *d = usb_desc_alloc_iface(sizeof(*d)); - - if (!d) { - return 1; // Not enough space in the configuration descriptor + if ((info->iface_num >= USBD_MAX_NUM_INTERFACES) || (info->iface_num < usb_config_desc->bNumInterfaces)) { + return -1; // Invalid interface number + } + if (((info->ep_in & USB_EP_DIR_MSK) == 0) || ((info->ep_out & USB_EP_DIR_MSK) != 0)) { + return -2; // Invalid endpoints + } + if ((info->rx_buffer == NULL) || (info->report_desc == NULL)) { + return -3; // Invalid buffers } - if ((info->iface_num < usb_config_desc->bNumInterfaces) || - (info->iface_num >= USBD_MAX_NUM_INTERFACES) || - ((info->ep_in & 0x80) == 0) || - ((info->ep_out & 0x80) != 0) || - (info->rx_buffer == NULL)) { + usb_hid_descriptor_block_t *d = usb_desc_alloc_iface(sizeof(usb_hid_descriptor_block_t)); - return 1; // Invalid configuration values + if (!d) { + return -4; // Not enough space in the configuration descriptor } // Interface descriptor @@ -61,7 +62,7 @@ int usb_hid_add(const usb_hid_info_t *info) { d->ep_out.bInterval = info->polling_interval; // Config descriptor - usb_desc_add_iface(sizeof(*d)); + usb_desc_add_iface(sizeof(usb_hid_descriptor_block_t)); // Interface state usb_iface_t *i = &usb_ifaces[info->iface_num]; @@ -78,31 +79,49 @@ int usb_hid_add(const usb_hid_info_t *info) { } int usb_hid_can_read(uint8_t iface_num) { - return ((iface_num < USBD_MAX_NUM_INTERFACES) && - (usb_ifaces[iface_num].type == USB_IFACE_TYPE_HID) && - (usb_ifaces[iface_num].hid.rx_buffer_len > 0) && - (usb_dev_handle.dev_state == USBD_STATE_CONFIGURED)); + if (usb_dev_handle.dev_state != USBD_STATE_CONFIGURED) { + return 0; // Device is not configured + } + if (iface_num >= USBD_MAX_NUM_INTERFACES) { + return 0; // Invalid interface number + } + if (usb_ifaces[iface_num].type != USB_IFACE_TYPE_HID) { + return 0; // Invalid interface type + } + if (usb_ifaces[iface_num].hid.rx_buffer_len == 0) { + return 0; // Nothing in the receiving buffer + } + return 1; } int usb_hid_can_write(uint8_t iface_num) { - return ((iface_num < USBD_MAX_NUM_INTERFACES) && - (usb_ifaces[iface_num].type == USB_IFACE_TYPE_HID) && - (usb_ifaces[iface_num].hid.in_idle) && - (usb_dev_handle.dev_state == USBD_STATE_CONFIGURED)); + if (usb_dev_handle.dev_state != USBD_STATE_CONFIGURED) { + return 0; // Device is not configured + } + if (iface_num >= USBD_MAX_NUM_INTERFACES) { + return 0; // Invalid interface number + } + if (usb_ifaces[iface_num].type != USB_IFACE_TYPE_HID) { + return 0; // Invalid interface type + } + if (usb_ifaces[iface_num].hid.in_idle == 0) { + return 0; // Last transmission is not over yet + } + return 1; } int usb_hid_read(uint8_t iface_num, uint8_t *buf, uint32_t len) { if (iface_num >= USBD_MAX_NUM_INTERFACES) { - return -1; // Invalid interface number + return -1; // Invalid interface number } if (usb_ifaces[iface_num].type != USB_IFACE_TYPE_HID) { - return -2; // Invalid interface type + return -2; // Invalid interface type } usb_hid_state_t *state = &usb_ifaces[iface_num].hid; // Copy maximum possible amount of data and truncate the buffer length if (len < state->rx_buffer_len) { - return 0; // Not enough data in the read buffer + return 0; // Not enough data in the read buffer } len = state->rx_buffer_len; state->rx_buffer_len = 0; @@ -116,10 +135,10 @@ int usb_hid_read(uint8_t iface_num, uint8_t *buf, uint32_t len) { int usb_hid_write(uint8_t iface_num, const uint8_t *buf, uint32_t len) { if (iface_num >= USBD_MAX_NUM_INTERFACES) { - return -1; // Invalid interface number + return -1; // Invalid interface number } if (usb_ifaces[iface_num].type != USB_IFACE_TYPE_HID) { - return -2; // Invalid interface type + return -2; // Invalid interface type } usb_hid_state_t *state = &usb_ifaces[iface_num].hid; @@ -133,9 +152,9 @@ int usb_hid_read_blocking(uint8_t iface_num, uint8_t *buf, uint32_t len, uint32_ uint32_t start = HAL_GetTick(); while (!usb_hid_can_read(iface_num)) { if (HAL_GetTick() - start >= timeout) { - return 0; // Timeout + return 0; // Timeout } - __WFI(); // Enter sleep mode, waiting for interrupt + __WFI(); // Enter sleep mode, waiting for interrupt } return usb_hid_read(iface_num, buf, len); } @@ -144,9 +163,9 @@ int usb_hid_write_blocking(uint8_t iface_num, const uint8_t *buf, uint32_t len, uint32_t start = HAL_GetTick(); while (!usb_hid_can_write(iface_num)) { if (HAL_GetTick() - start >= timeout) { - return 0; // Timeout + return 0; // Timeout } - __WFI(); // Enter sleep mode, waiting for interrupt + __WFI(); // Enter sleep mode, waiting for interrupt } return usb_hid_write(iface_num, buf, len); } @@ -236,7 +255,7 @@ static int usb_hid_class_setup(USBD_HandleTypeDef *dev, usb_hid_state_t *state, } static uint8_t usb_hid_class_data_in(USBD_HandleTypeDef *dev, usb_hid_state_t *state, uint8_t ep_num) { - if ((ep_num | 0x80) == state->ep_in) { + if ((ep_num | USB_EP_DIR_IN) == state->ep_in) { // Ensure that the FIFO is empty before a new transfer, // this condition could be caused by a new transfer // before the end of the previous transfer. @@ -251,10 +270,10 @@ static uint8_t usb_hid_class_data_out(USBD_HandleTypeDef *dev, usb_hid_state_t * // enough for state->max_packet_len bytes. state->rx_buffer_len = USBD_LL_GetRxDataSize(dev, ep_num); - if (state->rx_buffer_len > 0) { - // Prepare the OUT EP to receive next packet - USBD_LL_PrepareReceive(dev, ep_num, state->rx_buffer, state->max_packet_len); + // Prepare the OUT EP to receive next packet + USBD_LL_PrepareReceive(dev, ep_num, state->rx_buffer, state->max_packet_len); + if (state->rx_buffer_len > 0) { // Block the OUT EP until we process received data usb_ep_set_nak(dev, ep_num); }