From 717ddbac55c62138a1b0fcb088793e0b49a681c2 Mon Sep 17 00:00:00 2001
From: Konstantin Kondrashov <konstantin@espressif.com>
Date: Mon, 6 Jul 2026 15:07:06 +0300
Subject: [PATCH] fix(esp_event): use recursive mutex API in handler unregister
 (SEC-220)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 components/esp_event/esp_event.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c
index 33a5c76423a..60a89234f8b 100644
--- a/components/esp_event/esp_event.c
+++ b/components/esp_event/esp_event.c
@@ -922,12 +922,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);
     }