From 26da5f564b7f72f71b61f4a74f4638be8d435323 Mon Sep 17 00:00:00 2001 From: Kris Bahnsen Date: Thu, 8 Feb 2024 01:22:03 -0800 Subject: [PATCH] furi/core/timer: resolve timer handle use-after-free post deletion (#3431) When xTimerDelete is called using a dymanic timer handle, the timer handle should immediately be considered unusable for any operation; including checking if the timer is still running. Under high system loads, that memory region may see fast reuse while furi_timer_free is sleeping between timer active checks. That reuse could result in memory at that pointer causing the timer active check to return true. Rework the furi_timer_delete process (in the case of dynamically allocated callback memory) to stop the timer, wait for it to stop, free the memory, and then delete the timer. Timers without dynamically allocated callback memory are just sent a delete command; no need to stop it first. Fixes: ff33bc6aea43 ("Furi: wait for timer wind down in destructor (#1716)") Signed-off-by: Kris Bahnsen --- furi/core/timer.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/furi/core/timer.c b/furi/core/timer.c index 027e4cf40..f667aae96 100644 --- a/furi/core/timer.c +++ b/furi/core/timer.c @@ -67,17 +67,22 @@ void furi_timer_free(FuriTimer* instance) { callb = (TimerCallback_t*)pvTimerGetTimerID(hTimer); - furi_check(xTimerDelete(hTimer, portMAX_DELAY) == pdPASS); - - while(furi_timer_is_running(instance)) furi_delay_tick(2); - if((uint32_t)callb & 1U) { + /* If callback memory was allocated, it is only safe to free it with + * the timer inactive. Send a stop command and wait for the timer to + * be in an inactive state. + */ + furi_check(xTimerStop(hTimer, portMAX_DELAY) == pdPASS); + while(furi_timer_is_running(instance)) furi_delay_tick(2); + /* Callback memory was allocated from dynamic pool, clear flag */ callb = (TimerCallback_t*)((uint32_t)callb & ~1U); /* Return allocated memory to dynamic pool */ free(callb); } + + furi_check(xTimerDelete(hTimer, portMAX_DELAY) == pdPASS); } FuriStatus furi_timer_start(FuriTimer* instance, uint32_t ticks) { @@ -170,4 +175,4 @@ void furi_timer_set_thread_priority(FuriTimerThreadPriority priority) { } else { furi_crash(); } -} \ No newline at end of file +}