From 55830f67e0648607d561c40ac1e41aa45f6f2bfc Mon Sep 17 00:00:00 2001 From: Sean McBride Date: Thu, 28 Dec 2023 18:18:13 -0500 Subject: [PATCH] Fix incorrect alignment in allocation in libusb_alloc_transfer After being suspicous of some code I was browsing, I tried changing PTR_ALIGN to align to 4096 bytes instead of just to pointer size (8 bytes), then enabled ASan then ran the xusb example, and it crashed. What ensued was a big code review of that function and related code. - reviewed use of macros like USBI_TRANSFER_TO_LIBUSB_TRANSFER - introduced new macros TRANSFER_PRIV_TO_USBI_TRANSFER and USBI_TRANSFER_TO_TRANSFER_PRIV to do pointer offset conversions - introduced some temporary variables, especially for USBI_TRANSFER_TO_LIBUSB_TRANSFER results - move some variable assignment and declaration together - replaced a few uses of PTR_ALIGN to instead use the alignment macros --- libusb/io.c | 48 +++++++++++++++++--------------------- libusb/libusbi.h | 18 ++++++++++++-- libusb/os/windows_common.c | 8 ++++--- 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/libusb/io.c b/libusb/io.c index 6e7d087d8..538f11ec6 100644 --- a/libusb/io.c +++ b/libusb/io.c @@ -1244,8 +1244,8 @@ void usbi_io_exit(struct libusb_context *ctx) static void calculate_timeout(struct usbi_transfer *itransfer) { - unsigned int timeout = - USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->timeout; + struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); + unsigned int timeout = transfer->timeout; if (!timeout) { TIMESPEC_CLEAR(&itransfer->timeout); @@ -1289,30 +1289,25 @@ DEFAULT_VISIBILITY struct libusb_transfer * LIBUSB_CALL libusb_alloc_transfer( int iso_packets) { - size_t priv_size; - size_t alloc_size; - unsigned char *ptr; - struct usbi_transfer *itransfer; - struct libusb_transfer *transfer; - assert(iso_packets >= 0); if (iso_packets < 0) return NULL; - priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size); - alloc_size = priv_size - + sizeof(struct usbi_transfer) - + sizeof(struct libusb_transfer) - + (sizeof(struct libusb_iso_packet_descriptor) * (size_t)iso_packets); - ptr = calloc(1, alloc_size); + size_t priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size); + size_t usbi_transfer_size = PTR_ALIGN(sizeof(struct usbi_transfer)); + size_t libusb_transfer_size = PTR_ALIGN(sizeof(struct libusb_transfer)); + size_t iso_packets_size = sizeof(struct libusb_iso_packet_descriptor) * (size_t)iso_packets; + size_t alloc_size = priv_size + usbi_transfer_size + libusb_transfer_size + iso_packets_size; + unsigned char *ptr = calloc(1, alloc_size); if (!ptr) return NULL; - itransfer = (struct usbi_transfer *)(ptr + priv_size); + struct usbi_transfer *itransfer = (struct usbi_transfer *)(ptr + priv_size); itransfer->num_iso_packets = iso_packets; itransfer->priv = ptr; usbi_mutex_init(&itransfer->lock); - transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); + struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); + return transfer; } @@ -1335,10 +1330,6 @@ struct libusb_transfer * LIBUSB_CALL libusb_alloc_transfer( */ void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer) { - struct usbi_transfer *itransfer; - size_t priv_size; - unsigned char *ptr; - if (!transfer) return; @@ -1346,13 +1337,12 @@ void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer) if (transfer->flags & LIBUSB_TRANSFER_FREE_BUFFER) free(transfer->buffer); - itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer); + struct usbi_transfer *itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer); usbi_mutex_destroy(&itransfer->lock); if (itransfer->dev) libusb_unref_device(itransfer->dev); - priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size); - ptr = (unsigned char *)itransfer - priv_size; + unsigned char *ptr = USBI_TRANSFER_TO_TRANSFER_PRIV(itransfer); assert(ptr == itransfer->priv); free(ptr); } @@ -1380,7 +1370,8 @@ static int arm_timer_for_next_timeout(struct libusb_context *ctx) /* act on first transfer that has not already been handled */ if (!(itransfer->timeout_flags & (USBI_TRANSFER_TIMEOUT_HANDLED | USBI_TRANSFER_OS_HANDLES_TIMEOUT))) { - usbi_dbg(ctx, "next timeout originally %ums", USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->timeout); + struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); + usbi_dbg(ctx, "next timeout originally %ums", transfer->timeout); return usbi_arm_timer(&ctx->timer, cur_ts); } } @@ -1442,8 +1433,9 @@ static int add_to_flying_list(struct usbi_transfer *itransfer) if (first && usbi_using_timer(ctx) && TIMESPEC_IS_SET(timeout)) { /* if this transfer has the lowest timeout of all active transfers, * rearm the timer with this transfer's timeout */ + struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); usbi_dbg(ctx, "arm timer for timeout in %ums (first in line)", - USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->timeout); + transfer->timeout); r = usbi_arm_timer(&ctx->timer, timeout); } #else @@ -2837,7 +2829,8 @@ void usbi_handle_disconnect(struct libusb_device_handle *dev_handle) to_cancel = NULL; usbi_mutex_lock(&ctx->flying_transfers_lock); for_each_transfer(ctx, cur) { - if (USBI_TRANSFER_TO_LIBUSB_TRANSFER(cur)->dev_handle == dev_handle) { + struct libusb_transfer *cur_transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(cur); + if (cur_transfer->dev_handle == dev_handle) { usbi_mutex_lock(&cur->lock); if (cur->state_flags & USBI_TRANSFER_IN_FLIGHT) to_cancel = cur; @@ -2852,8 +2845,9 @@ void usbi_handle_disconnect(struct libusb_device_handle *dev_handle) if (!to_cancel) break; + struct libusb_transfer *transfer_to_cancel = USBI_TRANSFER_TO_LIBUSB_TRANSFER(to_cancel); usbi_dbg(ctx, "cancelling transfer %p from disconnect", - (void *) USBI_TRANSFER_TO_LIBUSB_TRANSFER(to_cancel)); + (void *) transfer_to_cancel); usbi_mutex_lock(&to_cancel->lock); usbi_backend.clear_transfer_priv(to_cancel); diff --git a/libusb/libusbi.h b/libusb/libusbi.h index d0d44177c..ef4854d6c 100644 --- a/libusb/libusbi.h +++ b/libusb/libusbi.h @@ -563,8 +563,11 @@ void usbi_get_real_time(struct timespec *tp); * 2. struct usbi_transfer * 3. struct libusb_transfer (which includes iso packets) [variable size] * - * from a libusb_transfer, you can get the usbi_transfer by rewinding the - * appropriate number of bytes. + * You can convert between them with the macros: + * TRANSFER_PRIV_TO_USBI_TRANSFER + * USBI_TRANSFER_TO_TRANSFER_PRIV + * USBI_TRANSFER_TO_LIBUSB_TRANSFER + * LIBUSB_TRANSFER_TO_USBI_TRANSFER */ struct usbi_transfer { @@ -617,10 +620,21 @@ enum usbi_transfer_timeout_flags { USBI_TRANSFER_TIMED_OUT = 1U << 2, }; +#define TRANSFER_PRIV_TO_USBI_TRANSFER(transfer_priv) \ + ((struct usbi_transfer *) \ + ((unsigned char *)(transfer_priv) \ + + PTR_ALIGN(sizeof(*transfer_priv)))) + +#define USBI_TRANSFER_TO_TRANSFER_PRIV(itransfer) \ + ((unsigned char *) \ + ((unsigned char *)(itransfer) \ + - PTR_ALIGN(usbi_backend.transfer_priv_size))) + #define USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer) \ ((struct libusb_transfer *) \ ((unsigned char *)(itransfer) \ + PTR_ALIGN(sizeof(struct usbi_transfer)))) + #define LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer) \ ((struct usbi_transfer *) \ ((unsigned char *)(transfer) \ diff --git a/libusb/os/windows_common.c b/libusb/os/windows_common.c index 1c474eef1..5373db9b1 100644 --- a/libusb/os/windows_common.c +++ b/libusb/os/windows_common.c @@ -491,9 +491,10 @@ static unsigned __stdcall windows_iocp_thread(void *arg) continue; } - itransfer = (struct usbi_transfer *)((unsigned char *)transfer_priv + PTR_ALIGN(sizeof(*transfer_priv))); + itransfer = TRANSFER_PRIV_TO_USBI_TRANSFER(transfer_priv); + struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); usbi_dbg(ctx, "transfer %p completed, length %lu", - USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer), ULONG_CAST(num_bytes)); + transfer, ULONG_CAST(num_bytes)); usbi_signal_transfer_completion(itransfer); } @@ -805,8 +806,9 @@ static int windows_handle_transfer_completion(struct usbi_transfer *itransfer) else result = GetLastError(); + struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); usbi_dbg(ctx, "handling transfer %p completion with errcode %lu, length %lu", - USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer), ULONG_CAST(result), ULONG_CAST(bytes_transferred)); + transfer, ULONG_CAST(result), ULONG_CAST(bytes_transferred)); switch (result) { case NO_ERROR: