mirror of
https://github.com/espressif/esp-idf.git
synced 2026-09-22 13:01:16 +03:00
fix(esp_event): use recursive mutex API in handler unregister (SEC-220)
1) loop->mutex is created with xSemaphoreCreateRecursiveMutex(). FreeRTOS requires that recursive mutexes are only acquired and released with xSemaphoreTakeRecursive / xSemaphoreGiveRecursive. esp_event_handler_unregister_with_internal() used the non-recursive xSemaphoreTake(loop->mutex, 0) / xSemaphoreGive(loop->mutex) in the fast path. The non-recursive Take bypasses uxRecursiveCallCount bookkeeping; if the same task subsequently takes the mutex recursively (e.g. re-entry from a handler or a follow-up register), the call count drifts. The non-recursive Give then unconditionally drops the holder, allowing another task to acquire the mutex while the original task still believes it holds the lock — a full lock violation on the handler list leading to UAF and potential RCE on attacker-driven event floods. Fix: replace xSemaphoreTake/xSemaphoreGive with the Recursive variants in the fast (try-take with timeout 0) path of unregister_with_internal. 2) avoid use-after-free when unregistering handler from a callback The recursive try-lock introduced in SEC-220 succeeds re-entrantly when a handler unregisters itself from within its own callback, causing the handler node to be freed immediately while the dispatch loop still writes profiling counters to it after the callback returns. Route the in-callback case to the deferred cleanup path and only free directly once no dispatch is active. Closes SEC_220
This commit is contained in:
@@ -877,12 +877,26 @@ esp_err_t esp_event_handler_unregister_with_internal(esp_event_loop_handle_t eve
|
||||
/* remove the handler if the mutex is taken successfully.
|
||||
* otherwise it will be removed from the list later */
|
||||
esp_err_t res = ESP_FAIL;
|
||||
if (xSemaphoreTake(loop->mutex, 0) == pdTRUE) {
|
||||
res = loop_remove_handler(&remove_handler_ctx);
|
||||
xSemaphoreGive(loop->mutex);
|
||||
if (xSemaphoreTakeRecursive(loop->mutex, 0) == pdTRUE) {
|
||||
/* We got the mutex. Check whether we are currently inside a handler
|
||||
* callback for this loop (running_task is set while handler_execute()
|
||||
* is active). If we are, we MUST NOT free the handler node immediately
|
||||
* because handler_execute() will still write to handler->invoked /
|
||||
* handler->time (profiling) after the callback returns. Use the
|
||||
* deferred cleanup-event path instead so the node is only freed once
|
||||
* the current dispatch iteration has fully completed. */
|
||||
if (loop->running_task == xTaskGetCurrentTaskHandle()) {
|
||||
res = find_and_unregister_handler(&remove_handler_ctx);
|
||||
} else {
|
||||
res = loop_remove_handler(&remove_handler_ctx);
|
||||
}
|
||||
xSemaphoreGiveRecursive(loop->mutex);
|
||||
} else {
|
||||
/* Another task holds the mutex (e.g. the loop task is dispatching an
|
||||
* event). Wait until it is released; by that point running_task will
|
||||
* have been cleared, so direct removal is safe. */
|
||||
xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY);
|
||||
res = find_and_unregister_handler(&remove_handler_ctx);
|
||||
res = loop_remove_handler(&remove_handler_ctx);
|
||||
xSemaphoreGiveRecursive(loop->mutex);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user