[PATCH v3 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!

--
  v3: ntdll: Add tests for ret_len on NtQueryInformationThread HideFromDebugger
      ntdll: Write ret_len before checking length

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

[PATCH v3 1/2] ntdll: Write ret_len before checking length

[Dylan Donnell]

From: Dylan Donnell

---
 dlls/ntdll/unix/thread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

case ThreadHideFromDebugger:
+        if (ret_len) *ret_len = sizeof(BOOLEAN);
         if (length != sizeof(BOOLEAN)) return STATUS_INFO_LENGTH_MISMATCH;
         if (!data) return STATUS_ACCESS_VIOLATION;
         SERVER_START_REQ( get_thread_info )
@@ -2285,7 +2286,6 @@ NTSTATUS WINAPI NtQueryInformationThread( HANDLE handle, THREADINFOCLASS class,
             *(BOOLEAN*)data = !!(reply->flags & GET_THREAD_INFO_FLAG_DBG_HIDDEN);
         }
         SERVER_END_REQ;
-        if (ret_len) *ret_len = sizeof(BOOLEAN);
         return STATUS_SUCCESS;

case ThreadPriorityBoost:

[PATCH v3 2/2] ntdll: 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..30606c17681 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 == 1, "Expected ret_len == 1, 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 == 1, "Expected ret_len == 1, 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)]

The added tests are failing here on Windows 11:
```
info.c:3333: Test failed: Expected ret_len == 1, got deadbeef
info.c:3338: Test failed: Expected ret_len == 1, got deadbeef
```

That's both with 32 and 64 bit tests.

That's weird of course because the previous test with the same parameters but invalid return length address does give STATUS_ACCESS_VIOLATION, suggesting it accessed the *ret_len before check (for read??) but then if length check didn't succeed it didn't set output length.

Not sure what to do with it, I don't see the sane reason to, e. g., read from output ret_len paramater. Maybe we can fix the test (expecting 0xdeadbeef where Windows returns it) and add if (ret_len) *(voltaile BOOLEAN *)ret_len |= 0; at start of ThreadHideFromDebugger handling (adding a comment that the app (specifying which) depends on returning STATUS_ACCESS_VIOLATION on unaccessible ret_len before instead of moving actual assignment. Unless maybe it is possible to find out a sane reason to access *ret_len before setting it.

Also, first patch should be better named as "ntdll/tests: Add tests for ret_len on NtQueryInformationThread HideFromDebugger." (ming /tests and period in the end), also '.' in the end of the first patch name.