Skip to content

Commit 26ba141

Browse files
committed
darwin: fix use after free during reenumeration
When one process has two contexts to libusb, it may trigger a reenumeration while the other context accesses `*dpriv->device`. Adding a mutex to access `device` can be costly because it is used in every function. Instead, we maintain an invariant: the `darwin_cached_device` must be valid at all times. In particular, this means the fields `device` and `service` must not be stale or we will have a use after free triggerable through a race condition. Previously, in `darwin_devices_detached` during reenumeration, it was possible for `old_device->device` to be freed. We now defer this free to `darwin_get_cached_device` after a new `device` is written. Note that the order is important because we do not have locks, so we must free the old device after writing the new one. The same point applies to `service` which we take care to free after the new `service` is written. A caveat here is that even if we do not crash it is still possible for one context to be using an outdated `device` and `service` leading to an error return. This should be acceptable because it would be a similar situation to the device being detached during an API call.
1 parent e3af735 commit 26ba141

File tree

1 file changed

+15
-11
lines changed

1 file changed

+15
-11
lines changed

libusb/os/darwin_usb.c

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -408,13 +408,6 @@ static void darwin_devices_detached (void *ptr, io_iterator_t rem_devices) {
408408
* will deref if needed. */
409409
usbi_dbg (NULL, "detected device detached due to re-enumeration. sessionID: 0x%" PRIx64 ", locationID: 0x%" PRIx64,
410410
session, locationID);
411-
412-
/* the device object is no longer usable so go ahead and release it */
413-
if (old_device->device) {
414-
(*(old_device->device))->Release(old_device->device);
415-
old_device->device = NULL;
416-
}
417-
418411
is_reenumerating = true;
419412
} else {
420413
darwin_deref_cached_device (old_device);
@@ -1014,7 +1007,8 @@ static enum libusb_error darwin_get_cached_device(struct libusb_context *ctx, io
10141007
UInt64 sessionID = 0, parent_sessionID = 0;
10151008
UInt32 locationID = 0;
10161009
enum libusb_error ret = LIBUSB_SUCCESS;
1017-
usb_device_t **device;
1010+
usb_device_t **device, **old_device = NULL;
1011+
io_service_t old_service = IO_OBJECT_NULL;
10181012
UInt8 port = 0;
10191013

10201014
/* assuming sessionID != 0 normally (never seen it be 0) */
@@ -1042,6 +1036,8 @@ static enum libusb_error darwin_get_cached_device(struct libusb_context *ctx, io
10421036
if (new_device->location == locationID && new_device->in_reenumerate) {
10431037
usbi_dbg (ctx, "found cached device with matching location that is being re-enumerated");
10441038
*old_session_id = new_device->session;
1039+
old_device = new_device->device;
1040+
old_service = new_device->service;
10451041
break;
10461042
}
10471043

@@ -1084,9 +1080,6 @@ static enum libusb_error darwin_get_cached_device(struct libusb_context *ctx, io
10841080

10851081
/* initialize locks */
10861082
usbi_mutex_init (&new_device->capture_mutex);
1087-
} else {
1088-
/* release the ref to old device's service */
1089-
IOObjectRelease (new_device->service);
10901083
}
10911084

10921085
/* keep track of devices regardless of if we successfully enumerate them to
@@ -1100,6 +1093,17 @@ static enum libusb_error darwin_get_cached_device(struct libusb_context *ctx, io
11001093
/* retain the service */
11011094
IOObjectRetain (service);
11021095

1096+
/* release the old device */
1097+
if (old_device) {
1098+
(*old_device)->Release (old_device);
1099+
old_device = NULL;
1100+
}
1101+
1102+
/* release the ref to old device's service */
1103+
if (old_service) {
1104+
IOObjectRelease (old_service);
1105+
}
1106+
11031107
/* cache the device descriptor */
11041108
ret = darwin_cache_device_descriptor(ctx, new_device);
11051109
if (ret)

0 commit comments

Comments
 (0)