usb: don't hold the callback mutex during the callback
What the developer wrote
Authored by Jon Griffiths
usb: don't hold the callback mutex during the callback
Note that the callback is called via trigger_event() from a single thread
only and so holding the mutex while calling it is pointless. If the callback
triggers code which attempts to unset the callback (for example, during error
handling), then this will deadlock.
Instead, use the mutex only to protect reading and writing the callback and
context variables themselves.
What changed, and why it matters
This commit fixes a potential deadlock in the Blockstream Jade hardware wallet's USB mass-storage code. Previously, a protective lock was held while calling a user-provided callback function. If that callback tried to unregister itself (for example, during error handling), it would wait forever for the lock that was already held, freezing the USB task. The fix copies the callback details under the lock, releases the lock, and only then invokes the callback. It is a reliability and availability fix, not a direct theft-of-funds bug, but deadlocks in a hardware wallet's USB path can affect device responsiveness and error recovery.
Treat as a low-to-moderate reliability/security fix. Review whether any other callback dispatch sites in the codebase hold locks across callbacks. No immediate emergency response is warranted, but the patch should be included in the next firmware release and regression-tested for USB attach/detach and error-handling paths.
Security signals we found
Deadlock in callback dispatch path
Mutex held across user-supplied callback invocation
Reentrancy hazard in USB mass-storage event handling
Potential denial-of-service via unresponsive USB task
Fix narrows critical section and copies callback state before invocation
Evidence from the diff
In main/usbhmsc/usbhmsc.c, the callback semaphore was renamed to callback_mutex and its use was narrowed. usbstorage_register_callback() still takes the mutex to update registered_callback and callback_ctx. trigger_event() now takes the mutex only to copy the callback pointer and context into local variables, gives the mutex back, and then calls the copied callback outside the critical section. usbstorage_task() now takes the mutex before clearing the callback/context on exit. This eliminates a reentrant deadlock where a callback that calls usbstorage_register_callback(NULL, NULL) would block on the already-held semaphore. The change is defensive and corrects a concurrency bug in FreeRTOS task synchronization.
Changed components
main/usbhmsc/usbhmsc.cusbstorage_register_callback()trigger_event()usbstorage_task()usbstorage_init()Inspect captured patch +16 / −12
diff --git a/main/usbhmsc/usbhmsc.c b/main/usbhmsc/usbhmsc.c
index 9e0b65c..7d3d233 100644
--- a/main/usbhmsc/usbhmsc.c
+++ b/main/usbhmsc/usbhmsc.c
@@ -44,7 +44,7 @@ typedef enum {
static SemaphoreHandle_t main_task_semaphore = NULL;
static SemaphoreHandle_t aux_task_semaphore = NULL;
static SemaphoreHandle_t interface_semaphore = NULL;
-static SemaphoreHandle_t callback_semaphore = NULL;
+static SemaphoreHandle_t callback_mutex = NULL;
static TaskHandle_t main_task = NULL;
static bool volatile usb_device_installed = false;
@@ -62,21 +62,23 @@ static void* callback_ctx = NULL;
void usbstorage_register_callback(usbstorage_callback_t callback, void* ctx)
{
- JADE_ASSERT(callback_semaphore);
- JADE_SEMAPHORE_TAKE(callback_semaphore);
+ JADE_ASSERT(callback_mutex);
+ JADE_SEMAPHORE_TAKE(callback_mutex);
registered_callback = callback;
callback_ctx = ctx;
- JADE_SEMAPHORE_GIVE(callback_semaphore);
+ JADE_SEMAPHORE_GIVE(callback_mutex);
}
static void trigger_event(usbstorage_event_t event, uint8_t device_address)
{
- JADE_ASSERT(callback_semaphore);
- JADE_SEMAPHORE_TAKE(callback_semaphore);
- if (registered_callback != NULL) {
- registered_callback(event, device_address, callback_ctx);
+ JADE_ASSERT(callback_mutex);
+ JADE_SEMAPHORE_TAKE(callback_mutex);
+ usbstorage_callback_t callback = registered_callback;
+ void* ctx = callback_ctx;
+ JADE_SEMAPHORE_GIVE(callback_mutex);
+ if (callback != NULL) {
+ callback(event, device_address, ctx);
}
- JADE_SEMAPHORE_GIVE(callback_semaphore);
}
static void msc_event_cb(const msc_host_event_t* event, void* arg)
@@ -228,8 +230,10 @@ static void usbstorage_task(void* ignore)
}
}
}
+ JADE_SEMAPHORE_TAKE(callback_mutex);
registered_callback = NULL;
callback_ctx = NULL;
+ JADE_SEMAPHORE_GIVE(callback_mutex);
// This may fail if the user removes the device at the right time
if (requires_host_uninstall) {
@@ -264,15 +268,15 @@ void usbstorage_init(void)
JADE_ASSERT(!main_task_semaphore);
JADE_ASSERT(!aux_task_semaphore);
JADE_ASSERT(!interface_semaphore);
- JADE_ASSERT(!callback_semaphore);
+ JADE_ASSERT(!callback_mutex);
main_task_semaphore = xSemaphoreCreateBinary();
aux_task_semaphore = xSemaphoreCreateBinary();
interface_semaphore = xSemaphoreCreateMutex();
- callback_semaphore = xSemaphoreCreateMutex();
+ callback_mutex = xSemaphoreCreateMutex();
JADE_ASSERT(main_task_semaphore);
JADE_ASSERT(aux_task_semaphore);
JADE_ASSERT(interface_semaphore);
- JADE_ASSERT(callback_semaphore);
+ JADE_ASSERT(callback_mutex);
}
bool usbstorage_start(void)
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.