[PATCH v9 0/6] MR10419: ntdll: Fix thread stops inside __wine_syscall_dispatcher
When SIGUSR1 is called while running inside `__wine_syscall_dispatcher`, the CPU context is not in a consistent state. To ensure that the captured context is always consistent: * If the SIGUSR1 arrives while saving the context, set a flag and defer handling it until after the context is fully saved. * If the SIGUSR1 arrives while restoring the context, rewind and retry restoring the context from the beginning. The latter relies on context restore being idempotent. The only case where that is not true is when the instrumentation callback is enabled. To address this, first make that path idempotent by saving the user %rip into %r10 in paths that enable the instrumentation callback. This fixes a regression introduced by 0a5f7a710365 which started leaking bogus SP values, but also generally fixes all other register value leaks due to SIGUSR1 racing with the syscall dispatcher. Supersedes: !10232 (FYI, the threadctx test introduced in this MR is stricter and also checks %rip, which will fail with that previous MR). Fixes: 0a5f7a710365 ("ntdll: Switch to the user stack before restoring the %fs register.") \ Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59333 -- v9: gitlab: Run ntdll tests with win64 architecture. ntdll/tests: Abort wow64 tests early if wow64 is unavailable ntdll/tests: Add a test for interrupted syscall handling ntdll: Block SIGUSR1 when the thread context is being modified ntdll: Fix thread stops inside __wine_syscall_dispatcher https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
From: Hoshino Lina <lina@lina.yt> When an instrumentation callback is enabled, the callback replaces the return address and the original return address is stored in %r10. Instead of using an xchg, explicitly save %rip separately (we use the %r10 slot which is otherwise not restored in the instrumentation return path without CONTEXT_INTEGER) and load that prior to returning into the callback. This allows this codepath to be idempotent, which will be a requirement for correctness when we introduce rewinding the syscall return in the SIGUSR1 handler. Note that the instrumentation callback is incompatible with CONTEXT_INTEGER (since it would clobber r10), and the syscall return path behaves as such. The logic is therefore that r10 has to be set strictly if RESTORE_FLAGS_INSTRUMENTATION is set and CONTEXT_INTEGER is not set (in the syscall frame). --- dlls/ntdll/unix/signal_x86_64.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 7e644a6dd9f..3151e2af89d 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1196,6 +1196,9 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context ) frame->rsp = context->Rsp; frame->rip = context->Rip; frame->eflags = context->EFlags; + if ((frame->restore_flags & RESTORE_FLAGS_INSTRUMENTATION) && + !(frame->restore_flags & CONTEXT_INTEGER)) + frame->r10 = context->Rip; } if (flags & CONTEXT_FLOATING_POINT) { @@ -1713,6 +1716,8 @@ NTSTATUS call_user_apc_dispatcher( CONTEXT *context, unsigned int flags, ULONG_P frame->rsp = (ULONG64)stack; frame->rip = (ULONG64)pKiUserApcDispatcher; frame->restore_flags |= CONTEXT_CONTROL; + if (frame->restore_flags & RESTORE_FLAGS_INSTRUMENTATION) + frame->r10 = frame->rip; return status; } @@ -1757,6 +1762,8 @@ NTSTATUS call_user_exception_dispatcher( EXCEPTION_RECORD *rec, CONTEXT *context frame->rsp = (ULONG64)stack; frame->rip = (ULONG64)pKiUserExceptionDispatcher; frame->restore_flags |= CONTEXT_CONTROL; + if (frame->restore_flags & RESTORE_FLAGS_INSTRUMENTATION) + frame->r10 = frame->rip; return status; } @@ -2295,7 +2302,10 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext, siginfo_t *siginfo ) frame->rip = *(ULONG64 *)RSP_sig( sigcontext ); frame->eflags = EFL_sig(sigcontext); frame->restore_flags = CONTEXT_CONTROL; - if (instrumentation_callback) frame->restore_flags |= RESTORE_FLAGS_INSTRUMENTATION; + if (instrumentation_callback) { + frame->r10 = frame->rip; + frame->restore_flags |= RESTORE_FLAGS_INSTRUMENTATION; + } RCX_sig( sigcontext ) = (ULONG64)frame; RSP_sig( sigcontext ) += sizeof(ULONG64); @@ -2723,7 +2733,10 @@ static void sigsys_handler( int signal, siginfo_t *siginfo, void *sigcontext ) frame->rcx = RIP_sig(ucontext); frame->eflags = EFL_sig(ucontext); frame->restore_flags = 0; - if (instrumentation_callback) frame->restore_flags |= RESTORE_FLAGS_INSTRUMENTATION; + if (instrumentation_callback) { + frame->r10 = frame->rip; + frame->restore_flags |= RESTORE_FLAGS_INSTRUMENTATION; + } RCX_sig(ucontext) = (ULONG_PTR)frame; R11_sig(ucontext) = frame->eflags; if (EFL_sig(ucontext) & 0x100) @@ -3351,7 +3364,8 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "jz 3b\n\t" "testl $0x2,%edx\n\t" /* CONTEXT_INTEGER */ "jnz 1b\n\t" - "xchgq %r10,(%rsp)\n\t" + "movq %r10,(%rsp)\n\t" /* frame->rip */ + "movq 0x40(%rcx),%r10\n\t" /* frame->r10 (original 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 */ @@ -3412,6 +3426,11 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher_instrumentation, __ASM_CFI(".cfi_adjust_cfa_offset 8\n\t") "popq 0x80(%rcx)\n\t" __ASM_CFI(".cfi_adjust_cfa_offset -8\n\t") + /* Save %rip into the r10 save slot, which will be + * used in the instrumentation return path. + */ + "pushq 0x70(%rcx)\n\t" /* frame->rip */ + "popq 0x40(%rcx)\n\t" /* frame->r10 */ "movl $0x10000,0xb4(%rcx)\n\t" /* frame->restore_flags <- RESTORE_FLAGS_INSTRUMENTATION */ "jmp " __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_prolog_end") ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
From: Hoshino Lina <lina@lina.yt> When SIGUSR1 is called while running inside __wine_syscall_dispatcher, the CPU context is not in a consistent state. To ensure that the captured context is always consistent: - If the SIGUSR1 arrives while saving the context, set a flag and defer handling it until after the context is fully saved. - If the SIGUSR1 arrives while restoring the context, rewind and retry restoring the context from the beginning. The latter relies on context restore being idempotent. This is ensured by the previous commit (the only non-idempotent path was when instrumentation callbacks are enabled). This fixes a regression introduced by 0a5f7a710365 which started leaking bogus SP values, but also generally fixes all other register value leaks due to SIGUSR1 racing with the syscall dispatcher. Fixes: 0a5f7a710365 ("ntdll: Switch to the user stack before restoring the %fs register.") Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59333 --- dlls/ntdll/unix/signal_x86_64.c | 242 +++++++++++++++++++++++--------- 1 file changed, 177 insertions(+), 65 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 3151e2af89d..7126f272b8f 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -490,11 +490,12 @@ struct syscall_frame void *syscall_cfa; /* 00a8 */ DWORD syscall_id; /* 00b0 */ DWORD restore_flags; /* 00b4 */ - DWORD align[2]; /* 00b8 */ + ULONG64 retval; /* 00b8 */ XSAVE_FORMAT xsave; /* 00c0 */ DECLSPEC_ALIGN(64) XSAVE_AREA_HEADER xstate; /* 02c0 */ }; +C_ASSERT( offsetof( struct syscall_frame, retval ) == 0xb8 ); C_ASSERT( offsetof( struct syscall_frame, xsave ) == 0xc0 ); C_ASSERT( offsetof( struct syscall_frame, xstate ) == 0x2c0 ); C_ASSERT( sizeof( struct syscall_frame ) == 0x300); @@ -513,6 +514,8 @@ struct amd64_thread_data DWORD fs; /* 0338 WOW TEB selector */ DWORD mxcsr; /* 033c Unix-side mxcsr register */ char syscall_dispatch; /* 0340 */ + volatile char sigusr1_blocked; /* 0341 */ + volatile char sigusr1_pending; /* 0342 */ }; C_ASSERT( sizeof(struct amd64_thread_data) <= sizeof(((struct ntdll_thread_data *)0)->cpu_data) ); @@ -522,6 +525,8 @@ C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, ins C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, fs ) == 0x338 ); C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, mxcsr ) == 0x33c ); C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, syscall_dispatch ) == 0x340 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, sigusr1_blocked ) == 0x341 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, sigusr1_pending ) == 0x342 ); static inline struct amd64_thread_data *amd64_thread_data(void) { @@ -2637,6 +2642,66 @@ static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) abort_thread( 0 ); } +/********************************************************************** + * usr1_inside_syscall + * + * Shared code to handle SIGUSR1 within a syscall. + */ +static void usr1_inside_syscall(struct xcontext *context) +{ + struct syscall_frame *frame = get_syscall_frame(); + ULONG64 saved_compaction = 0; + I386_CONTEXT *wow_context; + + context->c.ContextFlags = CONTEXT_FULL | CONTEXT_SEGMENTS | CONTEXT_EXCEPTION_REQUEST; + + NtGetContextThread( GetCurrentThread(), &context->c ); + if (xstate_extended_features) + { + if (user_shared_data->XState.CompactionEnabled) + frame->xstate.CompactionMask |= xstate_extended_features; + context_init_xstate( &context->c, &frame->xstate ); + saved_compaction = frame->xstate.CompactionMask; + } + wait_suspend( &context->c ); + if (xstate_extended_features) frame->xstate.CompactionMask = saved_compaction; + if (context->c.ContextFlags & 0x40) + { + /* xstate is updated directly in frame's xstate */ + context->c.ContextFlags &= ~0x40; + frame->restore_flags |= 0x40; + } + if ((wow_context = get_cpu_area( IMAGE_FILE_MACHINE_I386 )) + && (wow_context->ContextFlags & CONTEXT_I386_CONTROL) == CONTEXT_I386_CONTROL) + { + WOW64_CPURESERVED *cpu = NtCurrentTeb()->TlsSlots[WOW64_TLS_CPURESERVED]; + + cpu->Flags |= WOW64_CPURESERVED_FLAG_RESET_STATE; + } + NtSetContextThread( GetCurrentThread(), &context->c ); +} + +/********************************************************************** + * deferred_sigusr1 + * + * Deferred handler for SIGUSR1 + * + * Called after syscall entry, on the kernel stack. + */ +void deferred_sigusr1(void) +{ + struct xcontext context; + + TRACE_(seh)( "handling deferred SIGUSR1\n"); + do { + amd64_thread_data()->sigusr1_pending = 0; + /* USR1 could arrive again immediately since we are not in signal context, + * so block it. */ + amd64_thread_data()->sigusr1_blocked++; + usr1_inside_syscall(&context); + amd64_thread_data()->sigusr1_blocked--; + } while (amd64_thread_data()->sigusr1_pending); +} /********************************************************************** * usr1_handler @@ -2646,54 +2711,37 @@ static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { ucontext_t *ucontext = init_handler( sigcontext ); + struct syscall_frame *frame = get_syscall_frame(); + struct xcontext *context; - if (is_inside_syscall( RSP_sig(ucontext) )) + extern const void *__wine_syscall_dispatcher_save_end_ptr; + extern const void *__wine_syscall_dispatcher_return_ptr; + extern const void *__wine_syscall_dispatcher_return_end_ptr; + if (RIP_sig(ucontext) >= (ULONG_PTR)__wine_syscall_dispatcher_instrumentation && + RIP_sig(ucontext) < (ULONG_PTR)__wine_syscall_dispatcher_save_end_ptr) { - struct syscall_frame *frame = get_syscall_frame(); - ULONG64 saved_compaction = 0; - I386_CONTEXT *wow_context; - struct xcontext *context; - - context = (struct xcontext *)(((ULONG_PTR)RSP_sig(ucontext) - 128 /* red zone */ - sizeof(*context)) & ~15); - if ((char *)context < (char *)ntdll_get_thread_data()->kernel_stack) - { - ERR_(seh)( "kernel stack overflow.\n" ); - return; - } - context->c.ContextFlags = CONTEXT_FULL | CONTEXT_SEGMENTS | CONTEXT_EXCEPTION_REQUEST; - if (frame->restore_flags & RESTORE_FLAGS_INCOMPLETE_FRAME_CONTEXT) - { - frame->restore_flags &= ~RESTORE_FLAGS_INCOMPLETE_FRAME_CONTEXT; - frame->eflags = 0x200; - fixup_frame_fpu_state( frame, ucontext ); - } - NtGetContextThread( GetCurrentThread(), &context->c ); - if (xstate_extended_features) - { - if (user_shared_data->XState.CompactionEnabled) - frame->xstate.CompactionMask |= xstate_extended_features; - context_init_xstate( &context->c, &frame->xstate ); - saved_compaction = frame->xstate.CompactionMask; - } - wait_suspend( &context->c ); - if (xstate_extended_features) frame->xstate.CompactionMask = saved_compaction; - if (context->c.ContextFlags & 0x40) - { - /* xstate is updated directly in frame's xstate */ - context->c.ContextFlags &= ~0x40; - frame->restore_flags |= 0x40; - } - if ((wow_context = get_cpu_area( IMAGE_FILE_MACHINE_I386 )) - && (wow_context->ContextFlags & CONTEXT_I386_CONTROL) == CONTEXT_I386_CONTROL) - { - WOW64_CPURESERVED *cpu = NtCurrentTeb()->TlsSlots[WOW64_TLS_CPURESERVED]; - - cpu->Flags |= WOW64_CPURESERVED_FLAG_RESET_STATE; - } - NtSetContextThread( GetCurrentThread(), &context->c ); + /* Interrupted during syscall entry. Defer the USR1. */ + TRACE_(seh)( "deferring SIGUSR1 during syscall entry (rip=0x%lx)\n", (long)RIP_sig(ucontext)); + amd64_thread_data()->sigusr1_pending = 1; + return; } - else + else if (RIP_sig(ucontext) >= (ULONG_PTR)__wine_syscall_dispatcher_return_ptr && + RIP_sig(ucontext) < (ULONG_PTR)__wine_syscall_dispatcher_return_end_ptr) { + /* Interrupted during syscall exit. Rewind the exit and handle as inside syscall. */ + TRACE_(seh)( "rewinding syscall exit due to SIGUSR1 (rip=0x%lx, rax=0x%lx)\n", + (long)RIP_sig(ucontext), (long)RAX_sig(ucontext)); + RAX_sig(ucontext) = (ULONG_PTR)frame->retval; /* Restore return value */ + R13_sig(ucontext) = (ULONG_PTR)NtCurrentTeb(); + RBP_sig(ucontext) = (ULONG_PTR)&frame->rbp; + RIP_sig(ucontext) = (ULONG_PTR)__wine_syscall_dispatcher_return_ptr; + RCX_sig(ucontext) = (ULONG_PTR)frame; + RSP_sig(ucontext) = (ULONG_PTR)frame; + /* Fallthrough */ + } + else if (!is_inside_syscall( RSP_sig(ucontext) )) + { + /* Interrupted outside a syscall. */ struct xcontext context; save_context( &context, ucontext ); @@ -2701,9 +2749,33 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) if (is_wow64() && context.c.SegCs == cs64_sel) context.c.ContextFlags |= CONTEXT_EXCEPTION_ACTIVE; wait_suspend( &context.c ); restore_context( &context, ucontext ); + return; } -} + else if (amd64_thread_data()->sigusr1_blocked) + { + /* Interrupted during a syscall that is accessing user context. */ + TRACE_(seh)( "deferring SIGUSR1 during critical syscall (rip=0x%lx, blocked=%d)\n", + (long)RIP_sig(ucontext), amd64_thread_data()->sigusr1_blocked); + amd64_thread_data()->sigusr1_pending = 1; + return; + } + /* Otherwise, interrupted within a syscall with a consistent user context. */ + context = (struct xcontext *)(((ULONG_PTR)RSP_sig(ucontext) - 128 /* red zone */ - sizeof(*context)) & ~15); + + if ((char *)context < (char *)ntdll_get_thread_data()->kernel_stack) + { + ERR_(seh)( "kernel stack overflow.\n" ); + return; + } + if (frame->restore_flags & RESTORE_FLAGS_INCOMPLETE_FRAME_CONTEXT) + { + frame->restore_flags &= ~RESTORE_FLAGS_INCOMPLETE_FRAME_CONTEXT; + frame->eflags = 0x200; + fixup_frame_fpu_state( frame, ucontext ); + } + usr1_inside_syscall(context); +} #if defined(__APPLE__) || defined(PR_SET_SYSCALL_USER_DISPATCH) /********************************************************************** @@ -2979,6 +3051,8 @@ void init_syscall_frame( LPTHREAD_START_ROUTINE entry, void *arg, BOOL suspend, assert( thread_data->frame_size == frame_size ); thread_data->instrumentation_callback = &instrumentation_callback; + thread_data->sigusr1_blocked = 0; + thread_data->sigusr1_pending = 0; #if defined __linux__ arch_prctl( ARCH_SET_GS, teb ); @@ -3104,6 +3178,28 @@ __ASM_GLOBAL_FUNC( signal_start_thread, /*********************************************************************** * __wine_syscall_dispatcher */ + +/* The instrumentation entry point comes first. This allows the range between + * __wine_syscall_dispatcher_instrumentation and __wine_syscall_dispatcher_save_end + * to be considered "entering a syscall" in the SIGUSR1 handler. + */ +__ASM_GLOBAL_FUNC( __wine_syscall_dispatcher_instrumentation, + "movq %gs:0x378,%rcx\n\t" /* thread_data->syscall_frame */ + "popq 0x70(%rcx)\n\t" /* frame->rip */ + __ASM_CFI(".cfi_adjust_cfa_offset -8\n\t") + __ASM_CFI_REG_IS_AT2(rip, rcx, 0xf0,0x00) + "pushfq\n\t" + __ASM_CFI(".cfi_adjust_cfa_offset 8\n\t") + "popq 0x80(%rcx)\n\t" + __ASM_CFI(".cfi_adjust_cfa_offset -8\n\t") + "movl $0x10000,0xb4(%rcx)\n\t" /* frame->restore_flags <- RESTORE_FLAGS_INSTRUMENTATION */ + /* Save %rip, into the r10 save slot, which will be + * used in the instrumentation return path. + */ + "pushq 0x70(%rcx)\n\t" /* frame->rip */ + "popq 0x40(%rcx)\n\t" /* frame->r10 */ + "jmp " __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_prolog_end") ) + __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_gs_load") ":\n\t" "movq %gs:0x378,%rcx\n\t" /* thread_data->syscall_frame */ @@ -3215,7 +3311,24 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "syscall\n\t" "leaq -0x98(%rbp),%rcx\n" #endif - "ldmxcsr 0x33c(%r13)\n\t" /* amd64_thread_data()->mxcsr */ + /* If the handler was interrupted while saving context, the + * SIGUSR1 is deferred. Handle it now. + */ + "\n" __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_save_end") ":\n\t" + "testl $1,0x342(%r13)\n\t" /* amd64_thread_data()->sigusr1_pending */ + "jz 1f\n\t" + "pushq %r8\n\t" + "pushq %r9\n\t" + "pushq %r10\n\t" + "sub $8,%rsp\n\t" /* align */ + "call " __ASM_NAME("deferred_sigusr1") "\n\t" + "add $8,%rsp\n\t" + "popq %r10\n\t" + "popq %r9\n\t" + "popq %r8\n\t" + "leaq -0x98(%rbp),%rcx\n" + + "1:\tldmxcsr 0x33c(%r13)\n\t" /* amd64_thread_data()->mxcsr */ "movl 0xb0(%rcx),%eax\n\t" /* frame->syscall_id */ "movq 0x18(%rcx),%r11\n\t" /* 2nd argument */ "movl %eax,%ebx\n\t" @@ -3249,6 +3362,16 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "movq 8(%r15),%r9\n\t" /* 6th argument */ "callq *%r12\n\t" "leaq -0x98(%rbp),%rcx\n\t" + + /* If the handler is interrupted between this point and + * __wine_syscall_dispatcher_return_end below, + * the syscall return will be restarted. Therefore, this + * code should be idempotent. Save the return value first, + * so it can be restored. The register state at this + * point must be restored in usr1_handler(). + */ + "movq %rax,0xb8(%rcx)\n\t" /* frame->retval */ + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return") ":\n\t" /* push rbp-based kernel stack cfi */ __ASM_CFI(".cfi_remember_state\n\t") @@ -3371,6 +3494,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "andq $~0x4000,(%rsp)\n\t" /* make sure NT flag is not set, or iretq will fault */ "popfq\n\t" "iretq\n" + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return_end") ":\n" /* pop rbp-based kernel stack cfi */ __ASM_CFI("\t.cfi_restore_state\n") @@ -3417,24 +3541,6 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher_return, "jnz " __ASM_LOCAL_LABEL("trace_syscall_ret" ) "\n\t" "jmp " __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return") ) -__ASM_GLOBAL_FUNC( __wine_syscall_dispatcher_instrumentation, - "movq %gs:0x378,%rcx\n\t" /* thread_data->syscall_frame */ - "popq 0x70(%rcx)\n\t" /* frame->rip */ - __ASM_CFI(".cfi_adjust_cfa_offset -8\n\t") - __ASM_CFI_REG_IS_AT2(rip, rcx, 0xf0,0x00) - "pushfq\n\t" - __ASM_CFI(".cfi_adjust_cfa_offset 8\n\t") - "popq 0x80(%rcx)\n\t" - __ASM_CFI(".cfi_adjust_cfa_offset -8\n\t") - /* Save %rip into the r10 save slot, which will be - * used in the instrumentation return path. - */ - "pushq 0x70(%rcx)\n\t" /* frame->rip */ - "popq 0x40(%rcx)\n\t" /* frame->r10 */ - "movl $0x10000,0xb4(%rcx)\n\t" /* frame->restore_flags <- RESTORE_FLAGS_INSTRUMENTATION */ - "jmp " __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_prolog_end") ) - - /*********************************************************************** * __wine_unix_call_dispatcher */ @@ -3562,5 +3668,11 @@ __ASM_GLOBAL_POINTER( __ASM_NAME("__wine_unix_call_dispatcher_prolog_end_ptr"), __ASM_LOCAL_LABEL("__wine_unix_call_dispatcher_prolog_end") ) __ASM_GLOBAL_POINTER( __ASM_NAME("__wine_unix_call_dispatcher_gs_load_ptr"), __ASM_LOCAL_LABEL("__wine_unix_call_dispatcher_gs_load") ) +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_syscall_dispatcher_save_end_ptr"), + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_save_end") ) +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_syscall_dispatcher_return_ptr"), + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return") ) +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_syscall_dispatcher_return_end_ptr"), + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return_end") ) #endif /* __x86_64__ */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
From: Hoshino Lina <lina@lina.yt> This fixes a race between threads calling NtSetContextThread()/NtGetContextThread() on themselves and SIGUSR1 arriving from the server. --- dlls/ntdll/unix/signal_x86_64.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 7126f272b8f..8ee58c9aac2 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1124,6 +1124,21 @@ void *get_wow_context( CONTEXT *context ) return get_cpu_area( IMAGE_FILE_MACHINE_I386 ); } +void deferred_sigusr1(void); + +static inline void block_sigusr1(void) +{ + amd64_thread_data()->sigusr1_blocked++; +} + +static inline void unblock_sigusr1(void) +{ + if (!--(amd64_thread_data()->sigusr1_blocked) && + amd64_thread_data()->sigusr1_pending) + { + deferred_sigusr1(); + } +} /*********************************************************************** * NtSetContextThread (NTDLL.@) @@ -1151,6 +1166,7 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context ) else flags &= ~CONTEXT_XSTATE; /* debug registers require a server call */ + block_sigusr1(); if (self && (flags & CONTEXT_DEBUG_REGISTERS)) self = (amd64_thread_data()->dr0 == context->Dr0 && amd64_thread_data()->dr1 == context->Dr1 && @@ -1158,6 +1174,7 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context ) amd64_thread_data()->dr3 == context->Dr3 && amd64_thread_data()->dr6 == context->Dr6 && amd64_thread_data()->dr7 == context->Dr7); + unblock_sigusr1(); if (!self) { @@ -1178,6 +1195,7 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context ) } } + block_sigusr1(); if (flags & CONTEXT_INTEGER) { frame->rax = context->Rax; @@ -1224,6 +1242,7 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context ) } frame->restore_flags |= flags & ~CONTEXT_INTEGER; + unblock_sigusr1(); return STATUS_SUCCESS; } @@ -1247,6 +1266,7 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) if (ret || !self) return ret; } + block_sigusr1(); if (needed_flags & CONTEXT_INTEGER) { context->Rax = frame->rax; @@ -1325,8 +1345,10 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) UINT64 mask; if (context_ex->XState.Length < sizeof(XSAVE_AREA_HEADER) || - context_ex->XState.Length > xstate_size) + context_ex->XState.Length > xstate_size) { + unblock_sigusr1(); return STATUS_INVALID_PARAMETER; + } if (user_shared_data->XState.CompactionEnabled) { @@ -1344,8 +1366,10 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) memset( xstate->Reserved2, 0, sizeof(xstate->Reserved2) ); if (xstate->Mask) { - if (context_ex->XState.Length < xstate_get_size( xstate->CompactionMask, xstate->Mask )) + if (context_ex->XState.Length < xstate_get_size( xstate->CompactionMask, xstate->Mask )) { + unblock_sigusr1(); return STATUS_BUFFER_OVERFLOW; + } copy_xstate( xstate, &frame->xstate, xstate->Mask ); /* copy_xstate may use avx in memcpy, restore xstate not to break the tests. */ frame->restore_flags |= CONTEXT_XSTATE; @@ -1361,6 +1385,7 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) amd64_thread_data()->dr6 = context->Dr6; amd64_thread_data()->dr7 = context->Dr7; } + unblock_sigusr1(); set_context_exception_reporting_flags( &context->ContextFlags, CONTEXT_SERVICE_ACTIVE ); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
From: Hoshino Lina <lina@lina.yt> This tests whether kernel-mode SP/IP values leak when __wine_syscall_dispatcher is interrupted by SIGUSR1, and whether NtGetContextThred()/NtSetContextThread() behave properly when interrupted by another thread. --- dlls/ntdll/tests/Makefile.in | 1 + dlls/ntdll/tests/threadctx.c | 220 +++++++++++++++++++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 dlls/ntdll/tests/threadctx.c diff --git a/dlls/ntdll/tests/Makefile.in b/dlls/ntdll/tests/Makefile.in index 42d663777ea..177b9c7e35c 100644 --- a/dlls/ntdll/tests/Makefile.in +++ b/dlls/ntdll/tests/Makefile.in @@ -25,6 +25,7 @@ SOURCES = \ testdll.c \ testdll.spec \ thread.c \ + threadctx.c \ threadpool.c \ time.c \ unwind.c \ diff --git a/dlls/ntdll/tests/threadctx.c b/dlls/ntdll/tests/threadctx.c new file mode 100644 index 00000000000..1858ce272de --- /dev/null +++ b/dlls/ntdll/tests/threadctx.c @@ -0,0 +1,220 @@ +/* + * Unit test suite for ntdll thread context behavior + * + * Copyright 2026 Hoshino Lina + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + * + */ + +#include <stdarg.h> +#include <stdbool.h> + +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winbase.h" +#include "winternl.h" +#include "wine/test.h" + +// 90 second max runtime, to avoid winetest timeouts +#define MAX_RUNTIME 90 +#define THREADS 50 +#define STACK_SIZE 0x10000 +#define LOOPS 2000 + +#ifdef __x86_64__ +#define CTX_SP Rsp +#define CTX_IP Rip +#endif +#ifdef __i386__ +#define CTX_SP Esp +#define CTX_IP Eip +#endif +#ifdef __arm__ +#define CTX_SP Sp +#define CTX_IP Pc +#endif +#ifdef __aarch64__ +#define CTX_SP Sp +#define CTX_IP Pc +#endif + +static volatile bool looping = true; + +struct thread { + DWORD tid; + bool stopped; + HANDLE hnd; + CONTEXT ctx; + DWORD64 stack_lo; + DWORD64 stack_hi; + NTSTATUS err; + bool complete; +}; + +struct thread t[THREADS]; + +static HMODULE hntdll; +static HMODULE hKernel32; +static NTSTATUS (WINAPI *pNtGetContextThread)(HANDLE,CONTEXT*); +static NTSTATUS (WINAPI *pNtSetContextThread)(HANDLE,CONTEXT*); +static void (WINAPI *pGetCurrentThreadStackLimits)(PULONG_PTR,PULONG_PTR); + +DWORD WINAPI thread_func(LPVOID lpParameter) +{ + struct thread *self = lpParameter; + + ULONG_PTR lo, hi; + GetCurrentThreadStackLimits(&lo, &hi); + + self->stack_lo = (DWORD64)lo; + self->stack_hi = (DWORD64)hi; + + + while (looping) { + CONTEXT c; + NTSTATUS status; + + /* Play with the FP context. These exercise the FP restore syscall + * path, which was seen to tickle a particular bug. + */ + c.ContextFlags = CONTEXT_FLOATING_POINT; + status = pNtGetContextThread( GetCurrentThread(), &c ); + if (status) + ok(!status, "Failed to get context: 0x%lx\n", (long)status); + c.ContextFlags = CONTEXT_FLOATING_POINT; + status = pNtSetContextThread( GetCurrentThread(), &c ); + if (status) + ok(!status, "Failed to set context: 0x%lx\n", (long)status); + } + + self->complete = true; + + return 0; +} + +static bool is_pe_map(DWORD64 addr) +{ + /* This check only makes sense on Wine, where fake kernel mode exists. */ + if (!winetest_platform_is_wine) + return true; + + /* See server/mapping.c for the upper limits. */ + return (addr >= 0x60000000 && addr < 0x7c000000) || + (addr >= 0x600000000000 && addr < 0x700000000000) || +#ifdef _WIN64 + (addr >= 0x140000000 && addr < 0x141000000); +#else + (addr >= 0x400000 && addr < 0x1400000); +#endif +} + +static void test_context_sp(void) +{ + int loop; + bool failed_sp = false; + bool failed_ip = false; + DWORD timeout = GetTickCount() + MAX_RUNTIME * 1000; + hntdll = GetModuleHandleA("ntdll.dll"); + hKernel32 = GetModuleHandleA("kernel32.dll"); + +#define X(f) p##f = (void*)GetProcAddress(hntdll, #f) + X(NtGetContextThread); + X(NtSetContextThread); +#undef X +#define X(f) p##f = (void*)GetProcAddress(hKernel32, #f) + X(GetCurrentThreadStackLimits); +#undef X + + for (int i = 0; i < THREADS; i++) { + t[i].hnd = CreateThread(0, STACK_SIZE, thread_func, (LPVOID)&t[i], 0, + &t[i].tid); + ok(!!t[i].hnd, "Failed to create thread"); + if (!t[i].hnd) { + looping = false; + break; + } + } + + Sleep(100); + + trace("Starting %d loops of thread context fetching\n", LOOPS); + + for (loop = 0; looping && loop < LOOPS && GetTickCount() < timeout; + loop++) { + for (int i = 0; i < THREADS; i++) { + if (SuspendThread(t[i].hnd) != (DWORD)-1) { + t[i].ctx.ContextFlags = CONTEXT_INTEGER | CONTEXT_CONTROL; + + if (GetThreadContext(t[i].hnd, &t[i].ctx)) { + bool in_sp_range = t[i].ctx.CTX_SP > t[i].stack_lo && + t[i].ctx.CTX_SP <= t[i].stack_hi; + bool in_ip_range = is_pe_map(t[i].ctx.CTX_IP); + + if (!in_sp_range) { + trace("[%d/%d:%d] SP=0x%llx [%llx..%llx]\n", loop, + LOOPS, i, (long long)t[i].ctx.CTX_SP, + (long long)t[i].stack_lo, + (long long)t[i].stack_hi); + failed_sp = true; + } + + if (!in_ip_range) { + trace("[%d/%d:%d] IP=0x%llx\n", loop, LOOPS, i, + (long long)t[i].ctx.CTX_IP); + failed_ip = true; + } + + t[i].stopped = true; + } else { + ResumeThread(t[i].hnd); + } + } + } + + for (int i = 0; i < THREADS; i++) { + if (t[i].stopped) + ResumeThread(t[i].hnd); + + t[i].stopped = false; + } + + if (failed_sp && failed_ip) + break; + } + + trace("Completed %d/%d loops\n", loop, LOOPS); + + looping = false; + + for (int i = 0; i < THREADS; i++) + WaitForSingleObject(t[i].hnd, INFINITE); + + ok(!failed_sp, "Invalid SP value detected"); + + /* This is known broken on i386 */ +#ifdef __i386__ + todo_wine +#endif + ok(!failed_ip, "Invalid IP value detected"); + + for (int i = 0; i < THREADS; i++) + ok(t[i].complete, "Thread %d died unexpectedly\n", i); +} + +START_TEST(threadctx) { + test_context_sp(); +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
From: Hoshino Lina <lina@lina.yt> If the tests are run with a pure wine64 build, then CreateProcessA will fail. Just stop the test early instead of carrying on failing every single check after that. --- dlls/ntdll/tests/exception.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index 5ba23d38d34..73835a444c6 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -4579,6 +4579,8 @@ static void test_wow64_context(void) sprintf( cmdline, "\"%s\" /c for /l %%n in () do @echo >nul", appname ); r = CreateProcessA( appname, cmdline, NULL, NULL, FALSE, CREATE_SUSPENDED, NULL, NULL, &si, &pi); ok( r, "failed to start %s err %lu\n", appname, GetLastError() ); + if (!r) + return; ret = pRtlWow64GetThreadContext( pi.hThread, &ctx ); ok(ret == STATUS_SUCCESS, "got %#lx\n", ret); @@ -9239,6 +9241,8 @@ static void test_debug_registers_wow64(void) si.cb = sizeof(si); bret = CreateProcessA(cmdline, NULL, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); ok(bret, "CreateProcessA failed\n"); + if (!bret) + return; bret = pIsWow64Process(pi.hProcess, &is_wow64); ok(bret && is_wow64, "expected Wow64 process\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
From: Hoshino Lina <lina@lina.yt> --- tools/gitlab/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/gitlab/test.yml b/tools/gitlab/test.yml index db77ad5111d..70d5e0afbbb 100644 --- a/tools/gitlab/test.yml +++ b/tools/gitlab/test.yml @@ -62,7 +62,7 @@ test-linux-64: extends: .wine-test variables: - INCLUDE_TESTS: "dinput ntoskrnl.exe" + INCLUDE_TESTS: "dinput ntdll ntoskrnl.exe" rules: - if: $CI_PIPELINE_SOURCE == 'merge_request_event' needs: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10419
Added a bunch of bug fixes (I was forgetting to save/restore some registers) and made the test stricter, which also now uncovers another issue where SIGUSR1 can race a thread setting/getting its own context. Added another commit to address that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10419#note_133277
participants (2)
-
Hoshino Lina -
Hoshino Lina (＠hoshinolina)