Skip to content

fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] (IEC-434) #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
roma-jam wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] (IEC-434) #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54c89ff
fix(usb_host_hid): Changed the handling of reallocating ctrl_xfer buf…
roma-jam Nov 13, 2025
87a6fec
fix(usb_host_hid): Harden hid_host_device_close() against concurrent …
roma-jam Nov 13, 2025
092f1b8
refactor(hid_host): Disable and release interface
roma-jam Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions host/class/hid/usb_host_hid/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,48 @@
## 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()`
- Fixed race condition in hid_host_device_close() that could lead to double-free and list corruption under concurrent close/disconnect

## [1.0.4] - 2025-09-24

### Added

- Added support for ESP32-H4

## 1.0.3
## [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
## [1.0.2] - 2024-01-25

### Added

- Added support for ESP32-P4
- Fixed device open procedure for HID devices with multiple non-sequential interfaces.

## 1.0.1
## [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

- 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
## [1.0.0] - 2023-06-22

### Added

- Initial version
137 changes: 90 additions & 47 deletions host/class/hid/usb_host_hid/hid_host.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -90,6 +95,7 @@ typedef enum {
HID_INTERFACE_STATE_IDLE, /**< HID Interface has been found in connected USB device */
HID_INTERFACE_STATE_READY, /**< HID Interface opened and ready to start transfer */
HID_INTERFACE_STATE_ACTIVE, /**< HID Interface is in use */
HID_INTERFACE_STATE_CLOSING, /**< HID Interface is in the process on closing */
HID_INTERFACE_STATE_WAIT_USER_DELETION, /**< HID Interface wait user to be removed */
HID_INTERFACE_STATE_MAX
} hid_iface_state_t;
Expand Down Expand Up @@ -398,7 +404,6 @@ static esp_err_t hid_host_add_interface(hid_device_t *hid_device,
*/
static esp_err_t _hid_host_remove_interface(hid_iface_t *iface)
{
iface->state = HID_INTERFACE_STATE_NOT_INITIALIZED;
STAILQ_REMOVE(&s_hid_driver->hid_ifaces_tailq, iface, hid_interface, tailq_entry);
free(iface);
return ESP_OK;
Expand Down Expand Up @@ -608,8 +613,6 @@ static esp_err_t hid_host_interface_release_and_free_transfer(hid_iface_t *iface

ESP_ERROR_CHECK( usb_host_transfer_free(iface->in_xfer) );

// Change state
iface->state = HID_INTERFACE_STATE_IDLE;
return ESP_OK;
}

Expand All @@ -628,18 +631,12 @@ static esp_err_t hid_host_disable_interface(hid_iface_t *iface)
ESP_ERR_NOT_FOUND,
"Interface handle not found");

HID_RETURN_ON_FALSE((HID_INTERFACE_STATE_ACTIVE == iface->state),
ESP_ERR_INVALID_STATE,
"Interface wrong state");

HID_RETURN_ON_ERROR( usb_host_endpoint_halt(iface->parent->dev_hdl, iface->ep_in),
"Unable to HALT EP");
HID_RETURN_ON_ERROR( usb_host_endpoint_flush(iface->parent->dev_hdl, iface->ep_in),
"Unable to FLUSH EP");
usb_host_endpoint_clear(iface->parent->dev_hdl, iface->ep_in);

iface->state = HID_INTERFACE_STATE_READY;

return ESP_OK;
}

Expand Down Expand Up @@ -771,33 +768,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)) {
esp_err_t ret;
const uint32_t ctrl_size = hid_device->ctrl_xfer->data_buffer_size;
const uint32_t required_size = USB_SETUP_PACKET_SIZE + req->wLength;

// Reallocate control transfer buffer if necessary
if (ctrl_size < required_size) {
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_LOGD(TAG, "Change HID ctrl xfer size from %"PRIu32" to %"PRIu32"", ctrl_size, 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_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 |
Expand All @@ -808,16 +812,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 (ctrl_xfer->actual_num_bytes < USB_SETUP_PACKET_SIZE) {
ret = ESP_ERR_INVALID_SIZE;
} else {
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 {
ret = ESP_ERR_INVALID_SIZE;
}
}
}

Expand Down Expand Up @@ -999,9 +1007,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();
Expand Down Expand Up @@ -1196,44 +1204,71 @@ esp_err_t hid_host_device_open(hid_host_device_handle_t hid_dev_handle,
esp_err_t hid_host_device_close(hid_host_device_handle_t hid_dev_handle)
{
hid_iface_t *hid_iface = get_iface_by_handle(hid_dev_handle);

HID_RETURN_ON_INVALID_ARG(hid_iface);

ESP_LOGD(TAG, "Close addr %d, iface %d, state %d",
hid_iface->dev_params.addr,
hid_iface->dev_params.iface_num,
hid_iface->state);

if (HID_INTERFACE_STATE_ACTIVE == hid_iface->state) {

hid_iface_state_t prev_state;
HID_ENTER_CRITICAL();
prev_state = hid_iface->state;

if (prev_state == HID_INTERFACE_STATE_CLOSING ||
prev_state == HID_INTERFACE_STATE_NOT_INITIALIZED) {
// Interface is already in the closing process or already closed
HID_EXIT_CRITICAL();
return ESP_OK;
}

if (prev_state == HID_INTERFACE_STATE_ACTIVE ||
prev_state == HID_INTERFACE_STATE_READY) {
hid_iface->state = HID_INTERFACE_STATE_CLOSING;
}

HID_EXIT_CRITICAL();

if (prev_state == HID_INTERFACE_STATE_ACTIVE) {
HID_RETURN_ON_ERROR( hid_host_disable_interface(hid_iface),
"Unable to disable HID Interface");
hid_iface->state = HID_INTERFACE_STATE_READY;
}

if (HID_INTERFACE_STATE_READY == hid_iface->state) {
if (prev_state == HID_INTERFACE_STATE_ACTIVE ||
prev_state == HID_INTERFACE_STATE_READY) {
HID_RETURN_ON_ERROR( hid_host_interface_release_and_free_transfer(hid_iface),
"Unable to release HID Interface");

// If the device is closing by user before device detached we need to flush user callback here
hid_iface->state = HID_INTERFACE_STATE_IDLE;
// Release Report Descriptor memory
free(hid_iface->report_desc);
hid_iface->report_desc = NULL;
}

// Now handle user_cb & list removal
bool need_user_callback = false;

HID_ENTER_CRITICAL();
if (hid_iface->user_cb && hid_iface->state != HID_INTERFACE_STATE_WAIT_USER_DELETION) {
// Let user handle the remove process and wait for next hid_host_device_close() call
// When user_cb is set, we do a two-step deletion
hid_iface->state = HID_INTERFACE_STATE_WAIT_USER_DELETION;
hid_host_user_interface_callback(hid_iface, HID_HOST_INTERFACE_EVENT_DISCONNECTED);
} else {
// Second call
need_user_callback = true;
} else if ((hid_iface->state == HID_INTERFACE_STATE_WAIT_USER_DELETION ||
hid_iface->user_cb == NULL) &&
hid_iface->state != HID_INTERFACE_STATE_NOT_INITIALIZED) {
// Second close OR no user callback at all AND not already removed: remove from list
Copy link
Collaborator

@tore-espressif tore-espressif Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that the '2 calls to close' was present even before. Can you explain why?

Copy link
Contributor Author

@roma-jam roma-jam Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look! This is still Draft / [WIP]. I’m landing a minimal bug fix first and prepared a draft as we discussed yesterday; if needed, I’ll follow up with a separate refactor to simplify the close logic.

On the “two calls to close”: yes, that existed before. When the upper layer opens the interface, it owns a handle. The first call closes the interface; the second removes the interface from the list and invalidates the handle so the upper layer never holds a dangling reference.

I’m keeping that behavior unchanged here to keep the fix small and low-risk. If you see anything blocking for the bug-fix itself, please feel free to share; otherwise I’ll ping when it’s ready for full review.

hid_iface->user_cb = NULL;
hid_iface->user_cb_arg = NULL;

/* Remove Interface from the list */
ESP_LOGD(TAG, "Remove addr %d, iface %d from list",
hid_iface->dev_params.addr,
hid_iface->dev_params.iface_num);
HID_ENTER_CRITICAL();
hid_iface->state = HID_INTERFACE_STATE_NOT_INITIALIZED;
_hid_host_remove_interface(hid_iface);
HID_EXIT_CRITICAL();
}
HID_EXIT_CRITICAL();

if (need_user_callback) {
// Inform the user and wait for next hid_host_device_close() call
hid_host_user_interface_callback(hid_iface, HID_HOST_INTERFACE_EVENT_DISCONNECTED);
}

return ESP_OK;
Expand Down Expand Up @@ -1337,7 +1372,15 @@ esp_err_t hid_host_device_stop(hid_host_device_handle_t hid_dev_handle)

HID_RETURN_ON_INVALID_ARG(iface);

return hid_host_disable_interface(iface);
// TODO: concurrent issue with hid_host_device_close(), add semaphore

esp_err_t ret = hid_host_disable_interface(iface);
if (ret != ESP_OK) {
return ret;
}
iface->state = HID_INTERFACE_STATE_READY;

return ESP_OK;
}

uint8_t *hid_host_get_report_descriptor(hid_host_device_handle_t hid_dev_handle,
Expand Down
Loading