From 76ddb69a3a2e70d757bc696daa6494346278d156 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Duval?= Date: Tue, 13 Dec 2022 17:37:40 +0100 Subject: [PATCH] usb_raw: abort transfers cleanly on kill, fix use-after-free * aborted transfers will release the notify semaphore when the cancel is notified. * the allocated buffer would be freed on return, while the usb stack eventually copied data in the buffer in our back, leading to KDL crashes, because the freed buffer would be right reallocated for some kernel team structures. * regression introduced by hrev55806, the transfers didn't need to be cancelled before. Change-Id: Ifb6e941f71d05c37c36f878059c33883bb72a67c Reviewed-on: https://review.haiku-os.org/c/haiku/+/5905 Reviewed-by: Adrien Destugues Reviewed-by: waddlesplash Tested-by: Commit checker robot --- headers/os/drivers/USB3.h | 4 ++++ src/add-ons/kernel/bus_managers/usb/usb.cpp | 16 +++++++++++++++- src/add-ons/kernel/drivers/bus/usb/usb_raw.cpp | 18 ++++++++++++------ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/headers/os/drivers/USB3.h b/headers/os/drivers/USB3.h index 9602e1b340..e02ba5a3e0 100644 --- a/headers/os/drivers/USB3.h +++ b/headers/os/drivers/USB3.h @@ -247,6 +247,10 @@ struct usb_module_info { uint8 portIndex); status_t (*disable_port)(usb_device hub, uint8 portIndex); + + /* Cancel all pending async requests in a device control pipe */ + status_t (*cancel_queued_requests)(usb_device device); + }; diff --git a/src/add-ons/kernel/bus_managers/usb/usb.cpp b/src/add-ons/kernel/bus_managers/usb/usb.cpp index 881d1e91bd..8a65f8f871 100644 --- a/src/add-ons/kernel/bus_managers/usb/usb.cpp +++ b/src/add-ons/kernel/bus_managers/usb/usb.cpp @@ -528,6 +528,19 @@ cancel_queued_transfers(usb_pipe pipe) } +status_t +cancel_queued_requests(usb_device dev) +{ + TRACE_MODULE("cancel_queued_requests(%" B_PRId32 ")\n", dev); + ObjectBusyReleaser object(gUSBStack->GetObject(dev)); + if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0) + return B_DEV_INVALID_PIPE; + Device *device = (Device *)object.Get(); + + return device->DefaultPipe()->CancelQueuedTransfers(false); +} + + status_t usb_ioctl(uint32 opcode, void *buffer, size_t bufferSize) { @@ -686,7 +699,8 @@ struct usb_module_info gModuleInfoV3 = { get_nth_child, // get_nth_child get_device_parent, // get_device_parent reset_port, // reset_port - disable_port // disable_port + disable_port, // disable_port + cancel_queued_requests // cancel_queued_requests //queue_bulk_v_physical // queue_bulk_v_physical }; diff --git a/src/add-ons/kernel/drivers/bus/usb/usb_raw.cpp b/src/add-ons/kernel/drivers/bus/usb/usb_raw.cpp index a07f8fe4b8..9ab7bf1930 100644 --- a/src/add-ons/kernel/drivers/bus/usb/usb_raw.cpp +++ b/src/add-ons/kernel/drivers/bus/usb/usb_raw.cpp @@ -731,14 +731,17 @@ usb_raw_ioctl(void *cookie, uint32 op, void *buffer, size_t length) } status = acquire_sem_etc(device->notify, 1, B_KILL_CAN_INTERRUPT, 0); - if (status != B_OK) - return status; + if (status != B_OK) { + gUSBModule->cancel_queued_requests(device->device); + acquire_sem(device->notify); + } command.control.status = device->status; command.control.length = device->actual_length; deviceLocker.Unlock(); - status = B_OK; + if (command.control.status == B_OK) + status = B_OK; if (inTransfer && user_memcpy(command.control.data, controlData, command.control.length) != B_OK) { status = B_BAD_ADDRESS; @@ -855,14 +858,17 @@ usb_raw_ioctl(void *cookie, uint32 op, void *buffer, size_t length) } status = acquire_sem_etc(device->notify, 1, B_KILL_CAN_INTERRUPT, 0); - if (status != B_OK) - return status; + if (status != B_OK) { + gUSBModule->cancel_queued_transfers(endpointInfo->handle); + acquire_sem(device->notify); + } command.transfer.status = device->status; command.transfer.length = device->actual_length; deviceLocker.Unlock(); - status = B_OK; + if (command.transfer.status == B_OK) + status = B_OK; if (op == B_USB_RAW_COMMAND_ISOCHRONOUS_TRANSFER) { if (user_memcpy(command.isochronous.packet_descriptors, packetDescriptors, descriptorsSize) != B_OK) {