fix(usb_host_hid): Changed the handling of reallocating ctrl_xfer buffer on large report descriptors

- Made all the null checks before any dereference
- Fixed the unlock path on usb_host_transfer_alloc() error
- Limited req->wLength to some sane maximum (2048 bytes)
- Enabled the tests for large and extra large Report Decriptor in pytest to run it in CI

Author: roma-jam

host/class/hid/usb_host_hid/CHANGELOG.md

@@ -1,21 +1,51 @@
-## 1.0.4
+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
+and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
+
+## [Unreleased]
+
+### Fixed
+
+- Fixed a vulnerability with overwrite freed heap memory during `hid_host_get_report_descriptor()`
+
+### Added
+
+- Added a limitation for the HID report descriptor size to a maximum of 2048 bytes
+
+## [1.0.4] - 2025-09-24
+
+### Added
+
 - Added support for ESP32-H4
 
-## 1.0.3
-- Fixed a bug with interface mismatch on EP IN transfer complete while several HID devices are present.
-- Fixed a bug during device freeing, while detaching one of several attached HID devices.
+## [1.0.3] - 2024-08-20
+
+### Fixed
+
+- Fixed a bug with interface mismatch on EP IN transfer complete while several HID devices are present
+- Fixed a bug during device freeing, while detaching one of several attached HID devices
+
+## [1.0.2] - 2024-01-25
 
-## 1.0.2
+### Added
 
 - Added support for ESP32-P4
-- Fixed device open procedure for HID devices with multiple non-sequential interfaces.
+- Fixed device open procedure for HID devices with multiple non-sequential interfaces
+
+## [1.0.1] - 2023-10-04
+
+### Added
+
+- Added `hid_host_get_device_info()` to get the basic information of a connected USB HID device
+
+### Fixed
 
-## 1.0.1
+- Fixed a bug where configuring the driver with `create_background_task = false` did not properly initialize the driver. This lead to `the hid_host_uninstall()` hang-up
+- Fixed a bug where `hid_host_uninstall()` would cause a crash during the call while USB device has not been removed
 
-- Fixed a bug where configuring the driver with `create_background_task = false` did not properly initialize the driver. This lead to `the hid_host_uninstall()` hang-up.
-- Fixed a bug where `hid_host_uninstall()` would cause a crash during the call while USB device has not been removed.
-- Added `hid_host_get_device_info()` to get the basic information of a connected USB HID device.
+## [1.0.0] - 2023-06-22
 
-## 1.0.0
+### Added
 
 - Initial version

host/class/hid/usb_host_hid/hid_host.c

@@ -19,6 +19,11 @@
 
 #include "usb/hid_host.h"
 
+// We are allowing realloc ctrl_xfer buffer, so max report desc size is limited by sane value
+// based on very large, exotic devices: can go into the low kilobytes
+#define HID_MIN_REPORT_DESC_LEN     512u
+#define HID_MAX_REPORT_DESC_LEN     2048u
+
 // HID spinlock
 static portMUX_TYPE hid_lock = portMUX_INITIALIZER_UNLOCKED;
 #define HID_ENTER_CRITICAL()    portENTER_CRITICAL(&hid_lock)
@@ -771,33 +776,40 @@ static esp_err_t hid_control_transfer(hid_device_t *hid_device,
  */
 static esp_err_t usb_class_request_get_descriptor(hid_device_t *hid_device, const hid_class_request_t *req)
 {
-    esp_err_t ret;
-    usb_transfer_t *ctrl_xfer = hid_device->ctrl_xfer;
-    const size_t ctrl_size = hid_device->ctrl_xfer->data_buffer_size;
-
     HID_RETURN_ON_INVALID_ARG(hid_device);
     HID_RETURN_ON_INVALID_ARG(hid_device->ctrl_xfer);
     HID_RETURN_ON_INVALID_ARG(req);
     HID_RETURN_ON_INVALID_ARG(req->data);
 
+    if (req->wLength > HID_MAX_REPORT_DESC_LEN) {
+        ESP_LOGE(TAG, "Requested descriptor size exceeds maximum");
+        return ESP_ERR_INVALID_SIZE;
+    }
+
     HID_RETURN_ON_ERROR( hid_device_try_lock(hid_device, DEFAULT_TIMEOUT_MS),
                          "HID Device is busy by other task");
 
-    if (ctrl_size < (USB_SETUP_PACKET_SIZE + req->wLength)) {
-        usb_device_info_t dev_info;
-        ESP_ERROR_CHECK(usb_host_device_info(hid_device->dev_hdl, &dev_info));
-        // reallocate the ctrl xfer buffer for new length
-        ESP_LOGD(TAG, "Change HID ctrl xfer size from %d to %d",
-                 (int) ctrl_size,
-                 (int) (USB_SETUP_PACKET_SIZE + req->wLength));
+    esp_err_t ret;
+    const size_t ctrl_size = hid_device->ctrl_xfer->data_buffer_size;
+    const size_t required_size = USB_SETUP_PACKET_SIZE + req->wLength;
+
+    // Reallocate control transfer buffer if necessary
+    if (ctrl_size < required_size) {
+        ESP_LOGD(TAG, "Change HID ctrl xfer size from %"PRIu32" to %"PRIu32"",
+                 (uint32_t) ctrl_size,
+                 (uint32_t) required_size);
 
         usb_host_transfer_free(hid_device->ctrl_xfer);
-        HID_RETURN_ON_ERROR( usb_host_transfer_alloc(USB_SETUP_PACKET_SIZE + req->wLength,
-                                                     0,
-                                                     &hid_device->ctrl_xfer),
-                             "Unable to allocate transfer buffer for EP0");
+
+        if (usb_host_transfer_alloc(required_size, 0, &hid_device->ctrl_xfer) != ESP_OK) {
+            ESP_LOGE(TAG, "Unable to re-allocate transfer buffer for EP0");
+            hid_device->ctrl_xfer = NULL;
+            hid_device_unlock(hid_device);
+            return ESP_ERR_NO_MEM;
+        }
     }
 
+    usb_transfer_t *ctrl_xfer = hid_device->ctrl_xfer;
     usb_setup_packet_t *setup = (usb_setup_packet_t *)ctrl_xfer->data_buffer;
 
     setup->bmRequestType = USB_BM_REQUEST_TYPE_DIR_IN |
@@ -808,16 +820,20 @@ static esp_err_t usb_class_request_get_descriptor(hid_device_t *hid_device, cons
     setup->wIndex = req->wIndex;
     setup->wLength = req->wLength;
 
-    ret = hid_control_transfer(hid_device,
-                               USB_SETUP_PACKET_SIZE + req->wLength,
-                               DEFAULT_TIMEOUT_MS);
+    ret = hid_control_transfer(hid_device, required_size, DEFAULT_TIMEOUT_MS);
 
-    if (ESP_OK == ret) {
-        ctrl_xfer->actual_num_bytes -= USB_SETUP_PACKET_SIZE;
-        if (ctrl_xfer->actual_num_bytes <= req->wLength) {
-            memcpy(req->data, ctrl_xfer->data_buffer + USB_SETUP_PACKET_SIZE, ctrl_xfer->actual_num_bytes);
-        } else {
+    if (ret == ESP_OK) {
+        if (ctrl_xfer->actual_num_bytes < USB_SETUP_PACKET_SIZE) {
             ret = ESP_ERR_INVALID_SIZE;
+        } else {
+            uint32_t response_len = ctrl_xfer->actual_num_bytes - USB_SETUP_PACKET_SIZE;
+            if (response_len <= req->wLength) {
+                memcpy(req->data,
+                       ctrl_xfer->data_buffer + USB_SETUP_PACKET_SIZE,
+                       response_len);
+            } else {
+                ret = ESP_ERR_INVALID_SIZE;
+            }
         }
     }
 
@@ -999,9 +1015,9 @@ esp_err_t hid_host_install_device(uint8_t dev_addr,
     /*
     * TIP: Usually, we need to allocate 'EP bMaxPacketSize0 + 1' here.
     * To take the size of a report descriptor into a consideration,
-    * we need to allocate more here, e.g. 512 bytes.
+    * we need to allocate more here.
     */
-    HID_GOTO_ON_ERROR(usb_host_transfer_alloc(512, 0, &hid_device->ctrl_xfer),
+    HID_GOTO_ON_ERROR(usb_host_transfer_alloc(HID_MIN_REPORT_DESC_LEN, 0, &hid_device->ctrl_xfer),
                       "Unable to allocate transfer buffer");
 
     HID_ENTER_CRITICAL();

host/class/hid/usb_host_hid/test_app/main/test_hid_basic.c

@@ -707,8 +707,7 @@ TEST_CASE("sudden_disconnect", "[hid_host]")
     vSemaphoreDelete(s_global_hdl_sem);
 }
 
-// TODO: Enable during the fix https://github.com/espressif/esp-usb/pull/320
-TEST_CASE("request Report Descriptor 32K", "[hid_host][ignore]")
+TEST_CASE("request Report Descriptor 32K", "[hid_host_extra_large_report]")
 {
     // Create semaphore for s_global_hdl, because it will be used in multiple tasks access
     s_global_hdl_sem = xSemaphoreCreateBinary();
@@ -756,7 +755,7 @@ TEST_CASE("mock_hid_device_with_large_report", "[hid_device_large_report][ignore
     }
 }
 
-TEST_CASE("mock_hid_device_with_32K_report", "[hid_device_large_report][ignore]")
+TEST_CASE("mock_hid_device_with_32K_report", "[hid_device_extra_large_report][ignore]")
 {
     hid_mock_device_set_mode(TEST_HID_MOCK_DEVICE_WITH_REPORT_DESC_32KB);
     hid_mock_device_run();

host/class/hid/usb_host_hid/test_app/pytest_usb_host_hid.py

@@ -33,13 +33,18 @@ def test_usb_host_hid(dut: Tuple[IdfDut, IdfDut]) -> None:
 
     host.run_all_single_board_cases(group='hid_host')
 
-    # TODO: Enable during the fix https://github.com/espressif/esp-usb/pull/320
-    # Expected to fail due to the overriding the freed memory during realloc in the HID Host driver
-
     # Tests with mocked HID device with large report descriptor (1905 bytes) for HID tests
-    # device.serial.hard_reset()
-    # device.expect_exact('Press ENTER to see the list of tests.')
-    # device.write('[hid_device_large_report]')
-    # device.expect_exact('HID mock device with large report descriptor has been started')
+    device.serial.hard_reset()
+    device.expect_exact('Press ENTER to see the list of tests.')
+    device.write('[hid_device_large_report]')
+    device.expect_exact('HID mock device with large report descriptor has been started')
+
+    host.run_all_single_board_cases(group='hid_host')
+
+    # Tests with mocked HID device with extra large report descriptor (32KB) for HID tests
+    device.serial.hard_reset()
+    device.expect_exact('Press ENTER to see the list of tests.')
+    device.write('[hid_device_extra_large_report]')
+    device.expect_exact('HID mock device with extra large report descriptor has been started')
 
-    # host.run_all_single_board_cases(group='hid_host')
+    host.run_all_single_board_cases(group='hid_host_extra_large_report')