[PATCH 1/3] ntdll: Refine NtSetInformationThread ThreadHideFromDebugger stubs.
CoD: Black Ops 3 and CoD: WWII do each one of these calls and terminate if they return unexpected results. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/tests/info.c | 27 +++++++++++++++++++++++++++ dlls/ntdll/unix/thread.c | 14 ++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c index cf3f7b9f6f2..eaf2f1a45b7 100644 --- a/dlls/ntdll/tests/info.c +++ b/dlls/ntdll/tests/info.c @@ -2346,6 +2346,7 @@ static void test_affinity(void) DWORD_PTR proc_affinity, thread_affinity; THREAD_BASIC_INFORMATION tbi; SYSTEM_INFO si; + ULONG dummy; GetSystemInfo(&si); status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessBasicInformation, &pbi, sizeof(pbi), NULL ); @@ -2450,6 +2451,32 @@ static void test_affinity(void) ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08x\n", status); ok( tbi.AffinityMask == (1 << si.dwNumberOfProcessors) - 1, "Unexpected thread affinity\n" ); + + dummy = 0; + status = pNtSetInformationThread( GetCurrentThread(), ThreadHideFromDebugger, &dummy, sizeof(ULONG) ); + ok( status == STATUS_INFO_LENGTH_MISMATCH, "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status ); + dummy = 0; + status = pNtSetInformationThread( GetCurrentThread(), ThreadHideFromDebugger, &dummy, 1 ); + ok( status == STATUS_INFO_LENGTH_MISMATCH, "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status ); + status = pNtSetInformationThread( (HANDLE)0xdeadbeef, ThreadHideFromDebugger, NULL, 0 ); + ok( status == STATUS_INVALID_HANDLE, "Expected STATUS_INVALID_HANDLE, got %08x\n", status ); + status = pNtSetInformationThread( GetCurrentThread(), ThreadHideFromDebugger, NULL, 0 ); + ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08x\n", status ); + dummy = 0; + status = NtQueryInformationThread( GetCurrentThread(), ThreadHideFromDebugger, &dummy, sizeof(ULONG), NULL ); + if (status == STATUS_INVALID_INFO_CLASS) + win_skip("ThreadHideFromDebugger not available\n"); + else + { + ok( status == STATUS_INFO_LENGTH_MISMATCH, "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status ); + dummy = 0; + status = NtQueryInformationThread( (HANDLE)0xdeadbeef, ThreadHideFromDebugger, &dummy, sizeof(ULONG), NULL ); + ok( status == STATUS_INFO_LENGTH_MISMATCH, "Expected STATUS_INFO_LENGTH_MISMATCH, got %08x\n", status ); + dummy = 0; + status = NtQueryInformationThread( GetCurrentThread(), ThreadHideFromDebugger, &dummy, 1, NULL ); + ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08x\n", status ); + if (status == STATUS_SUCCESS) ok( dummy == 1, "Expected dummy == 1, got %08x\n", dummy ); + } } static void test_NtGetCurrentProcessorNumber(void) diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index d26e0a98cac..2e2e5a3882d 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -831,6 +831,8 @@ NTSTATUS WINAPI NtQueryInformationThread( HANDLE handle, THREADINFOCLASS class, { NTSTATUS status; + TRACE("(%p,%d,%p,%x,%p)\n", handle, class, data, length, ret_len); + switch (class) { case ThreadBasicInformation: @@ -1063,6 +1065,14 @@ NTSTATUS WINAPI NtQueryInformationThread( HANDLE handle, THREADINFOCLASS class, #endif } + case ThreadHideFromDebugger: + if (length != sizeof(BOOLEAN)) return STATUS_INFO_LENGTH_MISMATCH; + if (!data) return STATUS_ACCESS_VIOLATION; + if (handle != GetCurrentThread()) return STATUS_ACCESS_DENIED; + *(BOOLEAN*)data = TRUE; + if (ret_len) *ret_len = sizeof(BOOLEAN); + return STATUS_SUCCESS; + case ThreadPriority: case ThreadBasePriority: case ThreadImpersonationToken: @@ -1088,6 +1098,8 @@ NTSTATUS WINAPI NtSetInformationThread( HANDLE handle, THREADINFOCLASS class, { NTSTATUS status; + TRACE("(%p,%d,%p,%x)\n", handle, class, data, length); + switch (class) { case ThreadZeroTlsCell: @@ -1152,6 +1164,8 @@ NTSTATUS WINAPI NtSetInformationThread( HANDLE handle, THREADINFOCLASS class, } case ThreadHideFromDebugger: + if (length) return STATUS_INFO_LENGTH_MISMATCH; + if (handle != GetCurrentThread()) return STATUS_INVALID_HANDLE; /* pretend the call succeeded to satisfy some code protectors */ return STATUS_SUCCESS; -- 2.27.0
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions. It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/signal_arm64.c | 4 ++-- dlls/ntdll/signal_i386.c | 4 ++-- dlls/ntdll/signal_x86_64.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/signal_arm64.c b/dlls/ntdll/signal_arm64.c index 0159888f7ab..10cbb7c131a 100644 --- a/dlls/ntdll/signal_arm64.c +++ b/dlls/ntdll/signal_arm64.c @@ -1358,12 +1358,12 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "brk #0; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "brk #0; ret\n\t.nops 16") /********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "brk #0; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "brk #0; ret\n\t.nops 16") /********************************************************************** * NtCurrentTeb (NTDLL.@) diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c index 21cc1b3ead4..e050b87257d 100644 --- a/dlls/ntdll/signal_i386.c +++ b/dlls/ntdll/signal_i386.c @@ -565,12 +565,12 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret\n\t.nops 16") /********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret\n\t.nops 16") /********************************************************************** * NtCurrentTeb (NTDLL.@) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index 52f7b73f8bf..3bae66be188 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -2751,11 +2751,11 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret\n\t.nops 64") /********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret\n\t.nops 64") #endif /* __x86_64__ */ -- 2.27.0
On 2020-06-22 23:27, Rémi Bernon wrote:
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions.
It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/signal_arm64.c | 4 ++-- dlls/ntdll/signal_i386.c | 4 ++-- dlls/ntdll/signal_x86_64.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/dlls/ntdll/signal_arm64.c b/dlls/ntdll/signal_arm64.c index 0159888f7ab..10cbb7c131a 100644 --- a/dlls/ntdll/signal_arm64.c +++ b/dlls/ntdll/signal_arm64.c @@ -1358,12 +1358,12 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "brk #0; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "brk #0; ret\n\t.nops 16")
/********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "brk #0; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "brk #0; ret\n\t.nops 16")
/********************************************************************** * NtCurrentTeb (NTDLL.@) diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c index 21cc1b3ead4..e050b87257d 100644 --- a/dlls/ntdll/signal_i386.c +++ b/dlls/ntdll/signal_i386.c @@ -565,12 +565,12 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret\n\t.nops 16")
/********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret\n\t.nops 16")
/********************************************************************** * NtCurrentTeb (NTDLL.@) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index 52f7b73f8bf..3bae66be188 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -2751,11 +2751,11 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret\n\t.nops 64")
/********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret\n\t.nops 64")
#endif /* __x86_64__ */
So apparently old binutils like what's used in default Proton build doesn't know about .nops, I guess it could be .fill, or possibly .align before and after the int. -- Rémi Bernon <rbernon(a)codeweavers.com>
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions. It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v2: Use .skip instead of .nops which doesn't work with old binutils, also fix the size on x86_64. dlls/ntdll/signal_arm64.c | 4 ++-- dlls/ntdll/signal_i386.c | 4 ++-- dlls/ntdll/signal_x86_64.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/signal_arm64.c b/dlls/ntdll/signal_arm64.c index 0159888f7ab..52d3d3f76ea 100644 --- a/dlls/ntdll/signal_arm64.c +++ b/dlls/ntdll/signal_arm64.c @@ -1358,12 +1358,12 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "brk #0; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "brk #0; ret\n\t.skip 16") /********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "brk #0; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "brk #0; ret\n\t.skip 16") /********************************************************************** * NtCurrentTeb (NTDLL.@) diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c index 21cc1b3ead4..abae17c0143 100644 --- a/dlls/ntdll/signal_i386.c +++ b/dlls/ntdll/signal_i386.c @@ -565,12 +565,12 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret\n\t.skip 16") /********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret\n\t.skip 16") /********************************************************************** * NtCurrentTeb (NTDLL.@) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index 52f7b73f8bf..ad2c010febc 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -2751,11 +2751,11 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer, /********************************************************************** * DbgBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgBreakPoint, 0, "int $3; ret\n\t.skip 16") /********************************************************************** * DbgUserBreakPoint (NTDLL.@) */ -__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret") +__ASM_STDCALL_FUNC( DbgUserBreakPoint, 0, "int $3; ret\n\t.skip 16") #endif /* __x86_64__ */ -- 2.27.0
Rémi Bernon <rbernon(a)codeweavers.com> writes:
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions.
It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Use .skip instead of .nops which doesn't work with old binutils, also fix the size on x86_64.
Is .skip truly portable? In winebuild we had to avoid it on macOS because it wasn't supported, I don't know if that's still true. -- Alexandre Julliard julliard(a)winehq.org
On 2020-06-24 09:13, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions.
It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Use .skip instead of .nops which doesn't work with old binutils, also fix the size on x86_64.
Is .skip truly portable? In winebuild we had to avoid it on macOS because it wasn't supported, I don't know if that's still true.
To be honest I have no idea, I guess I could send another version with .space instead, as it seems to be used in winebuild on macOS, and to be equivalent according to GNU as manual (except for HPPA targets, but that should not be of concern). -- Rémi Bernon <rbernon(a)codeweavers.com>
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 2020-06-24 09:13, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions.
It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Use .skip instead of .nops which doesn't work with old binutils, also fix the size on x86_64.
Is .skip truly portable? In winebuild we had to avoid it on macOS because it wasn't supported, I don't know if that's still true.
To be honest I have no idea, I guess I could send another version with .space instead, as it seems to be used in winebuild on macOS, and to be equivalent according to GNU as manual (except for HPPA targets, but that should not be of concern).
A bunch of explicit nops or int3 may be easier... -- Alexandre Julliard julliard(a)winehq.org
On 2020-06-24 09:38, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 2020-06-24 09:13, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions.
It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Use .skip instead of .nops which doesn't work with old binutils, also fix the size on x86_64.
Is .skip truly portable? In winebuild we had to avoid it on macOS because it wasn't supported, I don't know if that's still true.
To be honest I have no idea, I guess I could send another version with .space instead, as it seems to be used in winebuild on macOS, and to be equivalent according to GNU as manual (except for HPPA targets, but that should not be of concern).
A bunch of explicit nops or int3 may be easier...
Right, of course. I'll do that. -- Rémi Bernon <rbernon(a)codeweavers.com>
On Wed, Jun 24, 2020 at 09:13:40AM +0200, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
CoD: Black Ops 3 and CoD: WWII modify these (and several others) and expect to have enough space for a few instructions.
It then verifies later that the patches are still in place, and terminates if the byte sequence do not match. Having small symbols can make the patches to overlap and the check to fail.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Use .skip instead of .nops which doesn't work with old binutils, also fix the size on x86_64.
Is .skip truly portable? In winebuild we had to avoid it on macOS because it wasn't supported, I don't know if that's still true.
This works at least with Xcode 9 command-line tools (which is the last version to support 32-bit). Huw.
CoD: WWII writes to PEB->BeingDebugged field, so we cannot completely trust it, but we can double check with ProcessDebugPort. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/om.c | 5 ++++- dlls/ntdll/tests/exception.c | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/om.c b/dlls/ntdll/om.c index aaed9abaa93..992d05d4769 100644 --- a/dlls/ntdll/om.c +++ b/dlls/ntdll/om.c @@ -364,9 +364,12 @@ static LONG WINAPI invalid_handle_exception_handler( EXCEPTION_POINTERS *eptr ) /* Everquest 2 / Pirates of the Burning Sea hooks NtClose, so we need a wrapper */ NTSTATUS close_handle( HANDLE handle ) { + DWORD_PTR debug_port; NTSTATUS ret = unix_funcs->NtClose( handle ); - if (ret == STATUS_INVALID_HANDLE && handle && NtCurrentTeb()->Peb->BeingDebugged) + if (ret == STATUS_INVALID_HANDLE && handle && NtCurrentTeb()->Peb->BeingDebugged && + !FAILED(NtQueryInformationProcess( NtCurrentProcess(), ProcessDebugPort, &debug_port, + sizeof(debug_port), NULL)) && debug_port) { __TRY { diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index a5e6faa461a..411439f180f 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -3595,6 +3595,12 @@ START_TEST(exception) test_suspend_process(); test_unload_trace(); + /* Call of Duty WWII writes to BeingDebugged then closes an invalid handle, + * crashing the game if an exception is raised. */ + NtCurrentTeb()->Peb->BeingDebugged = 0x98; + test_closehandle(0, (HANDLE)0xdeadbeef); + NtCurrentTeb()->Peb->BeingDebugged = 0; + #elif defined(__x86_64__) #define X(f) p##f = (void*)GetProcAddress(hntdll, #f) @@ -3638,6 +3644,12 @@ START_TEST(exception) else skip( "Dynamic unwind functions not found\n" ); + /* Call of Duty WWII writes to BeingDebugged then closes an invalid handle, + * crashing the game if an exception is raised. */ + NtCurrentTeb()->Peb->BeingDebugged = 0x98; + test_closehandle(0, (HANDLE)0xdeadbeef); + NtCurrentTeb()->Peb->BeingDebugged = 0; + #endif VirtualFree(code_mem, 0, MEM_RELEASE); -- 2.27.0
participants (3)
-
Alexandre Julliard -
Huw Davies -
Rémi Bernon