-
Notifications
You must be signed in to change notification settings - Fork 49
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[ccid] Cherry-pick upstream fix for polling_transfer UaF (#1099)
Cherry-pick the fix we landed into the upstream CCID repository, fixing a possible use-after-free and memory corruption due to race conditions. One particular scenario seems to be card insertion/removal happening at roughly the same times as SCardConnect/SCardDisconnect calls from clients. This fixes #1098.
- Loading branch information
1 parent
8460ea3
commit 79ab957
Showing
2 changed files
with
188 additions
and
18 deletions.
There are no files selected for viewing
161 changes: 161 additions & 0 deletions
161
third_party/ccid/patches/0005-Fix-possible-use-after-free-of-polling_transfer.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| From 8f197d0dcc11387f7864ba26e9379b014b03e137 Mon Sep 17 00:00:00 2001 | ||
| From: Maksim Ivanov <emaxx@google.com> | ||
| Date: Tue, 17 Oct 2023 11:20:48 +0200 | ||
| Subject: [PATCH] Fix possible use-after-free of polling_transfer | ||
|
|
||
| Guard the multi-thread usage of polling_transfer with a mutex, to | ||
| prevent the race condition between InterruptStop() and | ||
| InterruptRead(): while the former is calling | ||
| libusb_cancel_transfer(), the latter might've already called | ||
| libusb_free_transfer() on the same pointer. | ||
|
|
||
| This replaces the atomic variables that were previously used | ||
| for the polling_transfer pointer, because atomic variables don't | ||
| prevent this kind of use-after-free race condition. | ||
|
|
||
| In practice, the probability of hitting the race condition was | ||
| presumably low, but the basic scenario is the | ||
| SCardConnect/SCardDisconnect calls overlapping with the time | ||
| moments when the card is inserted/removed. | ||
| --- | ||
| src/ccid_usb.c | 46 +++++++++++++++++++++++++++------------------- | ||
| 1 file changed, 27 insertions(+), 19 deletions(-) | ||
|
|
||
| diff --git a/src/ccid_usb.c b/src/ccid_usb.c | ||
| index c5d47c9a..391af65d 100644 | ||
| --- a/src/ccid_usb.c | ||
| +++ b/src/ccid_usb.c | ||
| @@ -117,7 +117,8 @@ typedef struct | ||
| _Atomic bool terminated; | ||
|
|
||
| /* libusb transfer for the polling (or NULL) */ | ||
| - _Atomic (struct libusb_transfer *) polling_transfer; | ||
| + pthread_mutex_t polling_transfer_mutex; | ||
| + struct libusb_transfer *polling_transfer; | ||
|
|
||
| /* pointer to the multislot extension (if any) */ | ||
| struct usbDevice_MultiSlot_Extension *multislot_extension; | ||
| @@ -745,7 +746,8 @@ status_t OpenUSBByName(unsigned int reader_index, /*@null@*/ char *device) | ||
| usbDevice[reader_index].real_nb_opened_slots = 1; | ||
| usbDevice[reader_index].nb_opened_slots = &usbDevice[reader_index].real_nb_opened_slots; | ||
| atomic_init(&usbDevice[reader_index].terminated, false); | ||
| - atomic_init(&usbDevice[reader_index].polling_transfer, NULL); | ||
| + pthread_mutex_init(&usbDevice[reader_index].polling_transfer_mutex, NULL); | ||
| + usbDevice[reader_index].polling_transfer = NULL; | ||
| usbDevice[reader_index].disconnected = false; | ||
|
|
||
| /* CCID common information */ | ||
| @@ -1115,6 +1117,8 @@ status_t CloseUSB(unsigned int reader_index) | ||
| usbDevice[reader_index].multislot_extension = NULL; | ||
| } | ||
|
|
||
| + pthread_mutex_destroy(&usbDevice[reader_index].polling_transfer_mutex); | ||
| + | ||
| if (usbDevice[reader_index].ccid.gemalto_firmware_features) | ||
| free(usbDevice[reader_index].ccid.gemalto_firmware_features); | ||
|
|
||
| @@ -1528,7 +1532,9 @@ int InterruptRead(int reader_index, int timeout /* in ms */) | ||
| return IFD_COMMUNICATION_ERROR; | ||
| } | ||
|
|
||
| - atomic_store(&usbDevice[reader_index].polling_transfer, transfer); | ||
| + pthread_mutex_lock(&usbDevice[reader_index].polling_transfer_mutex); | ||
| + usbDevice[reader_index].polling_transfer = transfer; | ||
| + pthread_mutex_unlock(&usbDevice[reader_index].polling_transfer_mutex); | ||
|
|
||
| // The termination might've been requested by the other thread before the | ||
| // polling_transfer field was written. In that case, we have to cancel the | ||
| @@ -1559,7 +1565,9 @@ int InterruptRead(int reader_index, int timeout /* in ms */) | ||
| actual_length = transfer->actual_length; | ||
| ret = transfer->status; | ||
|
|
||
| - atomic_store(&usbDevice[reader_index].polling_transfer, NULL); | ||
| + pthread_mutex_lock(&usbDevice[reader_index].polling_transfer_mutex); | ||
| + usbDevice[reader_index].polling_transfer = NULL; | ||
| + pthread_mutex_unlock(&usbDevice[reader_index].polling_transfer_mutex); | ||
| libusb_free_transfer(transfer); | ||
|
|
||
| DEBUG_PERIODIC3("after (%d) (%d)", reader_index, ret); | ||
| @@ -1593,8 +1601,6 @@ int InterruptRead(int reader_index, int timeout /* in ms */) | ||
| ****************************************************************************/ | ||
| void InterruptStop(int reader_index) | ||
| { | ||
| - struct libusb_transfer *transfer; | ||
| - | ||
| /* Multislot reader: redirect to Multi_InterrupStop */ | ||
| if (usbDevice[reader_index].multislot_extension != NULL) | ||
| { | ||
| @@ -1607,17 +1613,17 @@ void InterruptStop(int reader_index) | ||
| // to be opposite to the one in InterruptRead() to avoid race conditions. | ||
| atomic_store(&usbDevice[reader_index].terminated, true); | ||
|
|
||
| - transfer = atomic_exchange(&usbDevice[reader_index].polling_transfer, | ||
| - NULL); | ||
| - if (transfer) | ||
| + pthread_mutex_lock(&usbDevice[reader_index].polling_transfer_mutex); | ||
| + if (usbDevice[reader_index].polling_transfer) | ||
| { | ||
| int ret; | ||
|
|
||
| - ret = libusb_cancel_transfer(transfer); | ||
| + ret = libusb_cancel_transfer(usbDevice[reader_index].polling_transfer); | ||
| if (ret < 0) | ||
| DEBUG_CRITICAL2("libusb_cancel_transfer failed: %s", | ||
| libusb_error_name(ret)); | ||
| } | ||
| + pthread_mutex_unlock(&usbDevice[reader_index].polling_transfer_mutex); | ||
| } /* InterruptStop */ | ||
|
|
||
|
|
||
| @@ -1668,8 +1674,9 @@ static void *Multi_PollingProc(void *p_ext) | ||
| break; | ||
| } | ||
|
|
||
| - atomic_store(&usbDevice[msExt->reader_index].polling_transfer, | ||
| - transfer); | ||
| + pthread_mutex_lock(&usbDevice[msExt->reader_index].polling_transfer_mutex); | ||
| + usbDevice[msExt->reader_index].polling_transfer = transfer; | ||
| + pthread_mutex_unlock(&usbDevice[msExt->reader_index].polling_transfer_mutex); | ||
|
|
||
| completed = 0; | ||
| while (!completed && !msExt->terminated) | ||
| @@ -1695,8 +1702,9 @@ static void *Multi_PollingProc(void *p_ext) | ||
| } | ||
| } | ||
|
|
||
| - atomic_store(&usbDevice[msExt->reader_index].polling_transfer, | ||
| - NULL); | ||
| + pthread_mutex_lock(&usbDevice[msExt->reader_index].polling_transfer_mutex); | ||
| + usbDevice[msExt->reader_index].polling_transfer = NULL; | ||
| + pthread_mutex_unlock(&usbDevice[msExt->reader_index].polling_transfer_mutex); | ||
|
|
||
| if (0 == rv) | ||
| { | ||
| @@ -1820,22 +1828,22 @@ static void *Multi_PollingProc(void *p_ext) | ||
| ****************************************************************************/ | ||
| static void Multi_PollingTerminate(struct usbDevice_MultiSlot_Extension *msExt) | ||
| { | ||
| - struct libusb_transfer *transfer; | ||
| - | ||
| if (msExt && !msExt->terminated) | ||
| { | ||
| msExt->terminated = true; | ||
|
|
||
| - transfer = atomic_exchange(&usbDevice[msExt->reader_index].polling_transfer, NULL); | ||
| + pthread_mutex_lock(&usbDevice[msExt->reader_index].polling_transfer_mutex); | ||
|
|
||
| - if (transfer) | ||
| + if (usbDevice[msExt->reader_index].polling_transfer) | ||
| { | ||
| int ret; | ||
|
|
||
| - ret = libusb_cancel_transfer(transfer); | ||
| + ret = libusb_cancel_transfer(usbDevice[msExt->reader_index].polling_transfer); | ||
| if (ret < 0) | ||
| DEBUG_CRITICAL2("libusb_cancel_transfer failed: %d", ret); | ||
| } | ||
| + | ||
| + pthread_mutex_unlock(&usbDevice[msExt->reader_index].polling_transfer_mutex); | ||
| } | ||
| } /* Multi_PollingTerminate */ | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters