[PATCH 0/1] 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!

[PATCH 1/1] ntdll: Return STATUS_ACCESS_VIOLATION if ret_len is not writable

[Dylan Donnell]

From: Dylan Donnell

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

diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c
index b64a7dd40af..eb94b21032f 100644
--- a/dlls/ntdll/unix/thread.c
+++ b/dlls/ntdll/unix/thread.c
@@ -2040,9 +2040,23 @@ NTSTATUS WINAPI NtQueryInformationThread( HANDLE handle, THREADINFOCLASS class,
                                           void *data, ULONG length, ULONG *ret_len )
 {
     unsigned int status;
+    MEMORY_BASIC_INFORMATION memory_info;

TRACE("(%p,%d,%p,%x,%p)\n", handle, class, data, (int)length, ret_len);

+    if (ret_len)
+    {
+        /* check whether ret_len is writable */
+        if (NtQueryVirtualMemory( GetCurrentProcess(), ret_len, MemoryBasicInformation, &memory_info, sizeof(memory_info), NULL ) != STATUS_SUCCESS) 
+        {
+            return STATUS_ACCESS_VIOLATION;
+        }
+        if (!(memory_info.Protect & (PAGE_READWRITE | PAGE_WRITECOPY | PAGE_EXECUTE_READWRITE | PAGE_EXECUTE_WRITECOPY))) 
+        {
+            return STATUS_ACCESS_VIOLATION;
+        }
+    }
+
     switch (class)
     {
     case ThreadBasicInformation:

[Elizabeth Figura (＠zfigura)]

Under what circumstances is this necessary?

Normally we'll attempt to write to ret_len, fault, and then handle_syscall_fault() will catch this and return STATUS_ACCESS_VIOLATION from the syscall.

[Paul Gofman (＠gofman)]

We do not handle syscalls output parameters checks this way. If ret_len is not writeable and is being written to in ntdll Unix-side function (like NtQueryInformationThread) that will already result in returning STATUS_ACCESS_VIOLATION to the caller (the fault in syscall will be handled this way by segv_handler).

So something else is going wrong here. Which information class (class parameter) is that which is helped by this patch? I guess most likely one of the following is actually going on:
- the class requested is not implemented, thus ret_len is not written and STATUS_NOT_IMPLEMENTED is returned instead of STATUS_ACCESS_VIOLATION. The correct fix would be implementing the missing class or adding some sane semi-stub implementation which will be touching *ret_len;
- the class is implemented and STATUS_ACCESS_VIOLATION is correctly returned but before faulting on writing *ret_len NtQueryInformationThread changes some output data which is not supposed to change in this case. The correct fix would be setting *ret_len earlier.

Any of that would need a test added (maybe to dlls/ntdll/tests/info.c or elsewhere depending on which class is that and thus where it fits in most naturally).

[Dylan Donnell (＠dy-tea)]

The class is implemented, but it is also called with an invalid length, and the ret_len write access violation should take precedence over the length mismatch.

[Paul Gofman (＠gofman)]

On Fri Mar 14 17:58:21 2025 +0000, Dylan Donnell wrote:
> The class is implemented, but it is also called with an invalid length,
> and the ret_len write access violation should take precedence over the
> length mismatch.

So the right way would probably be to add the test case for this class with such conditions and then set *ret_len before checking length and returning STATUS_INFO_LENGTH_MISMATCH or STATUS_BUFFER_TOO_SMALL.

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

[Paul Gofman (＠gofman)]

On Fri Mar 14 17:58:21 2025 +0000, Paul Gofman wrote:
> So the right way would probably be to add the test case for this class
> with such conditions and then set *ret_len before checking length and
> returning STATUS_INFO_LENGTH_MISMATCH or STATUS_BUFFER_TOO_SMALL.

Maybe it doesn't necessarily need to be tested with invalid ret_len address / access violation specifically, it looks reasonable to set *ret_len before returning STATUS_INFO_LENGTH_MISMATCH status, not doing that is probably an oversight.