[PATCH 0/5] MR8930: Fix check for early IRP cancellation
This matches https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/using-a-dr... and, as far as I can see, is the intended idea and what the comment implies. The idea, as I understand it, is to avoid a TOCTOU hazard by first setting the cancel routine and only afterwards checking the Cancel flag. If it's set while the previous cancel routine (returned by the second IoSetCancelRoutine call) is not NULL, that means it's been enabled before our first IoSetCancelRoutine call and we have to take care of it. Conversely, if there is no previous routine set, it must mean that ntoskrnl has removed the routine we just set and it's going to call it, or has already called it (and it's probably now trying to acquire the spinlock). In that case we leave handling the cancellation to the routine and do nothing special here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
From: Matteo Bruni <mbruni(a)codeweavers.com> Also adjust xinput to handle that. --- dlls/hidclass.sys/device.c | 2 +- dlls/xinput1_3/main.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index cb65391e443..852c8ebfbff 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -122,7 +122,7 @@ void hid_queue_remove_pending_irps( struct hid_queue *queue ) while ((irp = hid_queue_pop_irp( queue ))) { - irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Status = STATUS_DEVICE_NOT_CONNECTED; IoCompleteRequest( irp, IO_NO_INCREMENT ); } } diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c index 831874dea73..c1ad48e3fc4 100644 --- a/dlls/xinput1_3/main.c +++ b/dlls/xinput1_3/main.c @@ -573,7 +573,11 @@ static void read_controller_state(struct xinput_controller *controller) if (!GetOverlappedResult(controller->device, &controller->hid.read_ovl, &read_len, TRUE)) { if (GetLastError() == ERROR_OPERATION_ABORTED) return; - if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE) controller_destroy(controller, TRUE); + if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE || + GetLastError() == ERROR_DEVICE_NOT_CONNECTED) + { + controller_destroy(controller, TRUE); + } else ERR("Failed to read input report, GetOverlappedResult failed with error %lu\n", GetLastError()); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
From: Matteo Bruni <mbruni(a)codeweavers.com> This matches https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/using-a-dr... and, as far as I can see, is the intended idea and what the comment implies. The idea, as I understand it, is to avoid a TOCTOU hazard by first setting the cancel routine and only afterwards checking the Cancel flag. If it's set while the previous cancel routine (returned by the second IoSetCancelRoutine call) is not NULL, that means it's been enabled before our first IoSetCancelRoutine call and we have to take care of it. Conversely, if there is no previous routine set, it must mean that ntoskrnl has removed the routine we just set and it's going to call it, or has already called it (and it's probably now trying to acquire the spinlock). In that case we leave handling the cancellation to the routine and do nothing special here. --- dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index 852c8ebfbff..5b7a675db7f 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -163,7 +163,7 @@ static NTSTATUS hid_queue_push_irp( struct hid_queue *queue, IRP *irp ) KeAcquireSpinLock( &queue->lock, &irql ); IoSetCancelRoutine( irp, read_cancel_routine ); - if (irp->Cancel && !IoSetCancelRoutine( irp, NULL )) + if (irp->Cancel && IoSetCancelRoutine( irp, NULL )) { /* IRP was canceled before we set cancel routine */ InitializeListHead( &irp->Tail.Overlay.ListEntry ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/nsiproxy.sys/device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/nsiproxy.sys/device.c b/dlls/nsiproxy.sys/device.c index 3e806ca9835..a101f8d11ec 100644 --- a/dlls/nsiproxy.sys/device.c +++ b/dlls/nsiproxy.sys/device.c @@ -291,7 +291,7 @@ static NTSTATUS handle_send_echo( IRP *irp ) return status; } IoSetCancelRoutine( irp, icmp_echo_cancel ); - if (irp->Cancel && !IoSetCancelRoutine( irp, NULL )) + if (irp->Cancel && IoSetCancelRoutine( irp, NULL )) { /* IRP was canceled before we set cancel routine */ return STATUS_CANCELLED; @@ -366,7 +366,7 @@ static NTSTATUS nsiproxy_change_notification( IRP *irp ) EnterCriticalSection( &nsiproxy_cs ); IoSetCancelRoutine( irp, change_notification_cancel ); - if (irp->Cancel && !IoSetCancelRoutine( irp, NULL )) + if (irp->Cancel && IoSetCancelRoutine( irp, NULL )) { /* IRP was canceled before we set cancel routine */ InitializeListHead( &irp->Tail.Overlay.ListEntry ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/dinput/tests/driver_bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/dinput/tests/driver_bus.c b/dlls/dinput/tests/driver_bus.c index cf42a1bcf6e..e8338e49711 100644 --- a/dlls/dinput/tests/driver_bus.c +++ b/dlls/dinput/tests/driver_bus.c @@ -198,7 +198,7 @@ static NTSTATUS wait_queue_add_pending_locked( struct wait_queue *queue, IRP *ir if (queue->pending_wait) return STATUS_INVALID_PARAMETER; IoSetCancelRoutine( irp, wait_cancel_routine ); - if (irp->Cancel && !IoSetCancelRoutine( irp, NULL )) + if (irp->Cancel && IoSetCancelRoutine( irp, NULL )) return STATUS_CANCELLED; irp->Tail.Overlay.DriverContext[0] = queue; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/http.sys/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/http.sys/http.c b/dlls/http.sys/http.c index dfbbc1e0389..973ff91801d 100644 --- a/dlls/http.sys/http.c +++ b/dlls/http.sys/http.c @@ -973,7 +973,7 @@ static NTSTATUS http_receive_request(struct request_queue *queue, IRP *irp) TRACE("Queuing IRP %p.\n", irp); IoSetCancelRoutine(irp, http_receive_request_cancel); - if (irp->Cancel && !IoSetCancelRoutine(irp, NULL)) + if (irp->Cancel && IoSetCancelRoutine(irp, NULL)) { /* The IRP was canceled before we set the cancel routine. */ ret = STATUS_CANCELLED; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8930
participants (5)
-
Elizabeth Figura (＠zfigura) -
Huw Davies (＠huw)
-
Matteo Bruni
-
Matteo Bruni (＠Mystral)
-
Rémi Bernon