[PATCH 0/1] MR7947: ntdll: Make sure NT flag is not set before iretq in wine_syscall_dispatcher_return on x64.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/ntdll/tests/exception.c | 79 ++++++++++++++++++++++++++++++++- dlls/ntdll/unix/signal_x86_64.c | 5 +++ 2 files changed, 83 insertions(+), 1 deletion(-)
diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index 514ad218274..000c6fc6d08 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -5736,21 +5736,81 @@ static void test_instrumentation_callback(void) 0x41, 0xff, 0xe2, /* jmp *r10 */ };
+ static const BYTE call_func_nt_flag[] = + { + 0x56, /* push %rsi */ + 0x57, /* push %rdi */ + 0x48, 0x89, 0xce, /* mov %rcx,%rsi */ + 0x48, 0x89, 0xd7, /* mov %rdx,%rdi */ + + 0x9c, /* pushfq */ + 0x9c, /* pushfq */ + 0x48, 0x81, 0x0c, 0x24, 0x00, 0x40, 0x00, 0x00, /* orq $0x4000,(%rsp) */ + 0x9d, /* popfq */ + + 0x41, 0xff, 0xd0, /* call *%r8 */ + + 0x4c, 0x89, 0x1e, /* mov %r11,(%rsi) */ + 0x9c, /* pushfq */ + 0x5a, /* popq %rdx */ + 0x9d, /* popfq */ + 0x48, 0x89, 0x17, /* mov %rdx,(%rdi) */ + 0x5f, /* pop %rdi */ + 0x5e, /* pop %rsi */ + 0xc3, /* ret */ + }; + + static const BYTE call_NtSetContextThread_nt_flag[] = + { + 0x9c, /* pushfq */ + 0x48, 0x81, 0x0c, 0x24, 0x00, 0x40, 0x00, 0x00, /* orq $0x4000,(%rsp) */ + 0x9d, /* popfq */ + 0x41, 0xff, 0xd0, /* call *%r8 */ + 0xc3, /* ret */ + }; + + static const BYTE call_clear_nt_flag[] = + { + 0x9c, /* pushfq */ + 0x48, 0x81, 0x24, 0x24, 0xff, 0xbf, 0xff, 0xff, /* and $~0x4000,(%rsp) */ + 0x9d, /* popfq */ + 0xc3, /* ret */ + }; + + NTSTATUS (WINAPI *func_NtSetContextThread_nt_flag)(HANDLE, CONTEXT *, void *func); + NTSTATUS (WINAPI *func_nt_flag)(UINT64 *ret_r11, UINT64 *ret_rflags, void *func); struct instrumentation_callback_data curr_data, data; PROCESS_INSTRUMENTATION_CALLBACK_INFORMATION info; HMODULE ntdll = GetModuleHandleA( "ntdll.dll" ); + void (WINAPI *func_clear_nt_flag)(void); void *pLdrInitializeThunk; EXCEPTION_RECORD record; void *vectored_handler; unsigned int i, count; + ULONG64 r11, rflags; NTSTATUS status; HANDLE thread; CONTEXT ctx; HWND hwnd; LONG pass;
+ if (is_arm64ec) return;
+ func_nt_flag = (void *)((char *)code_mem + 512); + memcpy( func_nt_flag, call_func_nt_flag, sizeof(call_func_nt_flag) ); + + func_NtSetContextThread_nt_flag = (void *)((char *)code_mem + 1024); + memcpy( func_NtSetContextThread_nt_flag, call_NtSetContextThread_nt_flag, sizeof(call_NtSetContextThread_nt_flag) ); + + func_clear_nt_flag = (void *)((char *)code_mem + 1536); + memcpy( func_clear_nt_flag, call_clear_nt_flag, sizeof(call_clear_nt_flag) ); + + status = func_nt_flag( &r11, &rflags, NtFlushProcessWriteBuffers ); + ok( !status, "got %#lx.\n", status ); + ok( r11 & 0x4000, "got %#I64x.\n", r11 ); + ok( rflags & 0x4000, "got %#I64x.\n", rflags ); + memcpy( code_mem, instrumentation_callback, sizeof(instrumentation_callback) ); *(void **)((char *)code_mem + 4) = &curr_data.call_count; *(void **)((char *)code_mem + 26) = curr_data.call_data; @@ -5790,6 +5850,14 @@ static void test_instrumentation_callback(void) ok( status == STATUS_SUCCESS, "got %#lx.\n", status ); ok( data.call_count == 1, "got %u.\n", data.call_count );
+ init_instrumentation_data( &curr_data ); + status = func_nt_flag( &r11, &rflags, NtFlushProcessWriteBuffers ); + data = curr_data; + ok( !status, "got %#lx.\n", status ); + ok( data.call_count == 1, "got %u.\n", data.call_count ); + ok( r11 & 0x4000, "got %#I64x.\n", r11 ); + ok( rflags & 0x4000, "got %#I64x.\n", rflags ); + vectored_handler = AddVectoredExceptionHandler( TRUE, test_instrumentation_callback_handler ); ok( !!vectored_handler, "failed.\n" ); init_instrumentation_data( &curr_data ); @@ -5835,8 +5903,17 @@ static void test_instrumentation_callback(void) ok( data.call_count == 1, "got %u.\n", data.call_count ); ok( data.call_data[0].r10 == (void *)ctx.Rip, "got %p, expected %p.\n", data.call_data[0].r10, (void *)ctx.Rip ); init_instrumentation_data( &curr_data ); + ctx.EFlags |= 0x4000; + func_NtSetContextThread_nt_flag( GetCurrentThread(), &ctx, NtSetContextThread ); + } + else if (pass == 6) + { + data = curr_data; + pRtlCaptureContext( &ctx ); + todo_wine ok( !(ctx.EFlags & 0x4000), "got %#lx.\n", ctx.EFlags ); + if (ctx.EFlags & 0x4000) func_clear_nt_flag(); } - ok( pass == 5, "got %ld.\n", pass ); + ok( pass == 6, "got %ld.\n", pass ); RemoveVectoredExceptionHandler( vectored_handler );
apc_count = 0; diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 64494de55f4..3c9b3e9026b 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -2946,6 +2946,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "movq (%rsp),%rcx\n\t" /* frame->rip */ "pushq %r11\n\t" /* make sure that if trap flag is set the trap happens on the first instruction after iret */ + "andq $~0x4000,(%rsp)\n\t" /* make sure NT flag is not set, or iretq will fault */ "popfq\n\t" "iretq\n" /* CONTEXT_INTEGER */ @@ -2956,6 +2957,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "movq 0x40(%rcx),%r10\n\t" "movq 0x48(%rcx),%r11\n\t" "movq 0x10(%rcx),%rcx\n\t" + "pushfq\n\t" + "andq $~0x4000,(%rsp)\n\t" /* make sure NT flag is not set, or iretq will fault */ + "popfq\n\t" "iretq\n" /* RESTORE_FLAGS_INSTRUMENTATION */ "2:\tmovq %gs:0x348,%r10\n\t" /* amd64_thread_data()->instrumentation_callback */ @@ -2967,6 +2971,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "xchgq %r10,(%rsp)\n\t" "pushq %r11\n\t" /* make sure that if trap flag is set the trap happens on the first instruction after iret */ + "andq $~0x4000,(%rsp)\n\t" /* make sure NT flag is not set, or iretq will fault */ "popfq\n\t" "iretq\n\t"
This helps The Finals which stopped working after recent game update.
The todo is test is related to https://gitlab.winehq.org/wine/wine/-/merge_requests/283 . That MR is otherwise orhtogonal though, it relates to sanitizing flags in the context passed to NtContextThread, whlie this one concerns what happens when NT flag is set by app upon calling syscall.
It could maybe look reasonable to sanitize the flag on syscall entry, but then the test shows that returned r11 value and the flags (apart from NtSetContextThread) keep NT flag, so fixing up before iret avoids some complication and also avoids adding extra to default syscall path.
On Fri May 2 11:04:10 2025 +0000, Paul Gofman wrote:
This helps The Finals which stopped working after recent game update. The todo in test is related to https://gitlab.winehq.org/wine/wine/-/merge_requests/283 . That MR is otherwise orhtogonal though, it relates to sanitizing flags in the context passed to NtContextThread, whlie this one concerns what happens when NT flag is set by app upon calling syscall. It could maybe look reasonable to sanitize the flag on syscall entry, but then the test shows that returned r11 value and the flags (apart from NtSetContextThread) keep NT flag, so fixing up before iret avoids some complication and also avoids adding extra to default syscall path.
Since the iret slowpath is triggered, it means the game is either installing instrumentation context or setting context, isn't it? Merely entering the syscall with NT flag is not enough to trigger the bug.
If the slowpath is triggered by CONTEXT_CONTROL, it would be automatically handled by !283 since now the EFlags is overwritten by a sanitized value.
Otherwise, the game is either using instrumentation callback or setting CONTEXT_INTEGER *only* (w/o _CONTROL). Either case seems unlikely to me. If this is the case, maybe that needs to be documented as well as tested.
On Fri May 2 11:06:50 2025 +0000, Jinoh Kang wrote:
Since the iret slowpath is triggered, it means the game is either installing instrumentation context or setting context, isn't it? Merely entering the syscall with NT flag is not enough to trigger the bug. If the slowpath is triggered by CONTEXT_CONTROL, it would be automatically handled by !283 since now the EFlags is overwritten by a sanitized value. Otherwise, the game is either using instrumentation callback or setting CONTEXT_INTEGER *only* (w/o _CONTROL). Either case seems unlikely to me. If this is the case, maybe that needs to be documented (as well as tested to prevent regression).
Probably doesn't matter short term, juet curious if I was getting it right, since !283 would make clear-NT-at-iret redundant if my hypothesis is correct and possibly closer to native even. In any case, thanks for your work!
-
Jinoh Kang (＠iamahuman) -
Paul Gofman
-
Paul Gofman (＠gofman)