hotplug: fix callback use-after-free races#1861
Conversation
usbi_hotplug_exit() freed the hotplug callback list without holding hotplug_cbs_lock, while every other path operating on that list already takes the lock. The LIBUSB_HOTPLUG_ENUMERATE path also kept using the registered callback object after dropping the lock. If another thread deregistered and freed that callback, the enumerate loop could read freed memory. Keep the handle locally, look up the callback under the lock for each enumerated device, copy the callback data to the stack, and invoke the user callback from that copy after unlocking.
There was a problem hiding this comment.
Thanks for turning this around so quickly. It looks to me that both races are correctly addressed, and I'm happy with the approach. You even went further than the patch sketched for issue B, if I understand correctly.
I noticed that with your code, calling libusb_hotplug_deregister_callback() from within an ENUMERATE callback now stops enumeration at the next device. whereas before it kept firing for the remaining devices. I think this is more correct, but maybe it worth documenting it.
| int found = 0; | ||
|
|
||
| usbi_mutex_lock(&ctx->hotplug_cbs_lock); | ||
| for_each_hotplug_cb(ctx, hotplug_cb) { |
There was a problem hiding this comment.
the lookup reuses the outer hotplug_cb as its iterator. It's safe (unused afterward), but maybe not explicit enough when reading the code, and maybe not super-safe against refactoring.
What about using a dedicated value?
There was a problem hiding this comment.
Done. Documented the ENUMERATE deregistration behavior and used a dedicated iterator.
|
Thanks @smaeljaish771 , all good to me. |
I should have time this week to look this over too... |
5 participants
usbi_hotplug_exit() freed the hotplug callback list without holding hotplug_cbs_lock, while every other path operating on that list already takes the lock.
The LIBUSB_HOTPLUG_ENUMERATE path also kept using the registered callback object after dropping the lock. If another thread deregistered and freed that callback, the enumerate loop could read freed memory. Keep the handle locally, look up the callback under the lock for each enumerated device, copy the callback data to the stack, and invoke the user callback from that copy after unlocking.
Reformulating the hotplug iteration macros for Clang thread-safety analysis is left out, as it is an independent change.
Fixes: #1859