[PATCH v5 0/2] MR7579: ntdll: Return STATUS_ACCESS_VIOLATION if ret_len is not writable

[Dylan Donnell (＠dy-tea)]

Required for implementation correctness check in certain protection software.

Thanks to mkrsym1  for the patch!

--
  v5: ntdll/tests: Add tests for ret_len on NtQueryInformationThread HideFromDebugger.

https://gitlab.winehq.org/wine/wine/-/merge_requests/7579

[PATCH v5 1/2] ntdll: Return STATUS_ACCESS_VIOLATION from NtQueryInformationThread ThreadHideFromDebugger if *ret_len is not writable.

[Dylan Donnell]

From: Dylan Donnell

---
 dlls/ntdll/unix/thread.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c
index b64a7dd40af..d3ddcf13963 100644
--- a/dlls/ntdll/unix/thread.c
+++ b/dlls/ntdll/unix/thread.c
@@ -2275,6 +2275,12 @@ NTSTATUS WINAPI NtQueryInformationThread( HANDLE handle, THREADINFOCLASS class,
         return get_thread_wow64_context( handle, data, length );

case ThreadHideFromDebugger:
+        /* TP Shell Service depends on ThreadHideFromDebugger returning
+         * STATUS_ACCESS_VIOLATION if *ret_len is not writable, before
+         * any other checks. Despite the status, the variable does not
+         * actually seem to be written at that time. */
+        if (ret_len) *(volatile ULONG *)ret_len |= 0;
+
         if (length != sizeof(BOOLEAN)) return STATUS_INFO_LENGTH_MISMATCH;
         if (!data) return STATUS_ACCESS_VIOLATION;
         SERVER_START_REQ( get_thread_info )

[PATCH v5 2/2] ntdll/tests: Add tests for ret_len on NtQueryInformationThread HideFromDebugger.

[Dylan Donnell]

From: Dylan Donnell

---
 dlls/ntdll/tests/info.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c
index 5003a5b50c2..d24639b6b68 100644
--- a/dlls/ntdll/tests/info.c
+++ b/dlls/ntdll/tests/info.c
@@ -3274,7 +3274,7 @@ static void test_HideFromDebugger(void)
 {
     NTSTATUS status;
     HANDLE thread, stop_event;
-    ULONG dummy;
+    ULONG dummy, ret_len;

dummy = 0;
     status = pNtSetInformationThread( GetCurrentThread(), ThreadHideFromDebugger, &dummy, sizeof(ULONG) );
@@ -3321,6 +3321,27 @@ static void test_HideFromDebugger(void)
     ok( status == STATUS_SUCCESS, "got %#lx\n", status );
     ok( dummy == 1, "Expected dummy == 1, got %08lx\n", dummy );

+    status = NtQueryInformationThread( thread, ThreadHideFromDebugger, &dummy, 1, (ULONG *)1 );
+    ok( status == STATUS_ACCESS_VIOLATION, "Expected STATUS_ACCESS_VIOLATION, got %08lx\n", status );
+
+    status = NtQueryInformationThread( thread, ThreadHideFromDebugger, &dummy, 0, (ULONG *)1 );
+    ok( status == STATUS_ACCESS_VIOLATION, "Expected STATUS_ACCESS_VIOLATION, got %08lx\n", status );
+
+    ret_len = 0xdeadbeef;
+    status = NtQueryInformationThread( thread, ThreadHideFromDebugger, &dummy, 0, &ret_len );
+    ok( status == STATUS_INFO_LENGTH_MISMATCH, "Expected STATUS_INFO_LENGTH_MISMATCH, got %#lx\n", status );
+    ok( ret_len == 0xdeadbeef, "Expected ret_len == deadbeef, got %08lx\n", ret_len );
+
+    ret_len = 0xdeadbeef;
+    status = NtQueryInformationThread( (HANDLE)0xdeadbeef, ThreadHideFromDebugger, &dummy, 1, &ret_len );
+    ok( status == STATUS_INVALID_HANDLE, "Expected STATUS_INVALID_HANDLE, got %#lx\n", status );
+    ok( ret_len == 0xdeadbeef, "Expected ret_len == deadbeef, got %08lx\n", ret_len );
+
+    ret_len = 0xdeadbeef;
+    status = NtQueryInformationThread( thread, ThreadHideFromDebugger, &dummy, 1, &ret_len );
+    ok( status == STATUS_SUCCESS, "got %#lx\n", status );
+    ok( ret_len == 1, "Expected ret_len == 1, got %08lx\n", ret_len );
+
     SetEvent( stop_event );
     WaitForSingleObject( thread, INFINITE );
     CloseHandle( thread );

[Paul Gofman (＠gofman)]

I tested separately that the access violation with 0 length and invalid ret_length pointer is not due to invalid alignment (the tests succeeds on Win11 if using 0xdead0000 instead of '1'). Also I could not find any influence of input *ret_len value on the function status.

So I don't see any sane business we might have to *ret_len in NtQueryInformationThread to make such a change less artificial, that is probably some very specific Windows implementation detail or bug? While it is probably not all that surprising that some DRMs may be very picky about the function's exact behaviour, so maybe it can go in this way?

[PATCH v5 0/2] MR7579: ntdll: Return STATUS_ACCESS_VIOLATION if ret_len is not writable - approved

[Paul Gofman (＠gofman)]

This merge request was approved by Paul Gofman.