[PATCH 0/5] MR8930: Fix check for early IRP cancellation

[Matteo Bruni (＠Mystral)]

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.

[PATCH 1/5] hidclass: Set Status for pending IRPs of removed devices to STATUS_DEVICE_NOT_CONNECTED.

[Matteo Bruni]

From: Matteo Bruni

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;
     }

[PATCH 2/5] hidclass: Fix check for early IRP cancellation.

[Matteo Bruni]

From: Matteo Bruni

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 );

[PATCH 3/5] nsiproxy: Fix check for early IRP cancellation.

[Matteo Bruni]

From: Matteo Bruni

---
 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 );

[PATCH 4/5] dinput/tests: Fix check for early IRP cancellation.

[Matteo Bruni]

From: Matteo Bruni

---
 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;

[PATCH 5/5] http.sys: Fix check for early IRP cancellation.

[Matteo Bruni]

From: Matteo Bruni

---
 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;

[PATCH 0/5] MR8930: Fix check for early IRP cancellation - approved

[Elizabeth Figura (＠zfigura)]

This merge request was approved by Elizabeth Figura.

[PATCH 0/5] MR8930: Fix check for early IRP cancellation - approved

[Huw Davies (＠huw)]

This merge request was approved by Huw Davies.

[PATCH 0/5] MR8930: Fix check for early IRP cancellation - approved

[Rémi Bernon (＠rbernon)]

This merge request was approved by Rémi Bernon.