diff options
author | Sean McBride <sean@rogue-research.com> | 2023-12-28 18:18:13 -0500 |
---|---|---|
committer | Tormod Volden <debian.tormod@gmail.com> | 2024-01-19 19:37:17 +0100 |
commit | 56d8f3c558dad4eaf0d571c054d44d07ace2331f (patch) | |
tree | 6e37561f20ee4744ccca1d7794d13e7c83da85f5 | |
parent | 20fb7513bcca472528aafc5a09baafc22c6e04a6 (diff) | |
download | libusb-56d8f3c558dad4eaf0d571c054d44d07ace2331f.tar.gz |
io: 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
In particular, libusb_alloc_transfer() was not using PTR_ALIGN()
on struct usbi_transfer and could allocate a wrong size.
Closes #1418
-rw-r--r-- | libusb/io.c | 48 | ||||
-rw-r--r-- | libusb/libusbi.h | 18 | ||||
-rw-r--r-- | libusb/os/windows_common.c | 8 | ||||
-rw-r--r-- | libusb/version_nano.h | 2 |
4 files changed, 43 insertions, 33 deletions
diff --git a/libusb/io.c b/libusb/io.c index 36e0b81..6348f8a 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 @@ out: 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 5c65dc2..b90ddbe 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 1c474ee..5373db9 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: diff --git a/libusb/version_nano.h b/libusb/version_nano.h index 744c167..bbd8bd5 100644 --- a/libusb/version_nano.h +++ b/libusb/version_nano.h @@ -1 +1 @@ -#define LIBUSB_NANO 11860 +#define LIBUSB_NANO 11861 |