[PATCH 0/2] MR4683: ntdll: Restore fs32_sel only if pthread TEB was not set already in init_handler.
Fixes spurious crashes in Steam when downloading games. The download causes a huge amount of SIGUSR1 signals, and it becomes very likely that one signal will be received while being inside the syscall or unix call dispatchers. When this happens, it can be received within the small range of instructions where %fs has been restored but we have not yet switched to the syscall stack, or the other way around in the return path. The signal handler then was restoring the 32bit %fs while returning to the syscall dispatcher, then we are entering a syscall with %fs set to the wrong value. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4683
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/signal_x86_64.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 0c291633ac6..3ac226a74dd 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1802,6 +1802,7 @@ static inline BOOL handle_interrupt( ucontext_t *sigcontext, EXCEPTION_RECORD *r rec->NumberParameters = 1; rec->ExceptionInformation[0] = context->Rcx; NtRaiseException( rec, context, FALSE ); + leave_handler( sigcontext ); return TRUE; case 0x2c: rec->ExceptionCode = STATUS_ASSERTION_FAILURE; @@ -2160,6 +2161,7 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) NtGetContextThread( GetCurrentThread(), &context.c ); wait_suspend( &context.c ); NtSetContextThread( GetCurrentThread(), &context.c ); + leave_handler( sigcontext ); } else { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4683
From: Rémi Bernon <rbernon(a)codeweavers.com> Fixes spurious crashes in Steam when downloading games. The download causes a huge amount of SIGUSR1 signals, and it becomes very likely that one signal will be received while being inside the syscall or unix call dispatchers. When this happens, it can be received within the small range of instructions where %fs has been restored but we have not yet switched to the syscall stack, or the other way around in the return path. The signal handler then was restoring the 32bit %fs while returning to the syscall dispatcher, then we are entering a syscall with %fs set to the wrong value. --- dlls/ntdll/unix/signal_x86_64.c | 74 ++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 33 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 3ac226a74dd..65f055d198d 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -816,13 +816,15 @@ static inline void set_sigcontext( const CONTEXT *context, ucontext_t *sigcontex /*********************************************************************** * init_handler */ -static inline ucontext_t *init_handler( void *sigcontext ) +static inline ucontext_t *init_handler( ucontext_t *sigcontext, BOOL *restore_fs ) { #ifdef __linux__ if (fs32_sel) { struct ntdll_thread_data *thread_data = (struct ntdll_thread_data *)&get_current_teb()->GdiTebBatch; - arch_prctl( ARCH_SET_FS, ((struct amd64_thread_data *)thread_data->cpu_data)->pthread_teb ); + void *new_fs = ((struct amd64_thread_data *)thread_data->cpu_data)->pthread_teb, *old_fs; + arch_prctl( ARCH_GET_FS, &old_fs ); + if ((*restore_fs = old_fs != new_fs)) arch_prctl( ARCH_SET_FS, new_fs ); } #endif return sigcontext; @@ -832,11 +834,10 @@ static inline ucontext_t *init_handler( void *sigcontext ) /*********************************************************************** * leave_handler */ -static inline void leave_handler( ucontext_t *sigcontext ) +static inline void leave_handler( ucontext_t *sigcontext, BOOL restore_fs ) { #ifdef __linux__ - if (fs32_sel && !is_inside_signal_stack( (void *)RSP_sig(sigcontext )) && !is_inside_syscall(sigcontext)) - __asm__ volatile( "movw %0,%%fs" :: "r" (fs32_sel) ); + if (fs32_sel && restore_fs) __asm__ volatile( "movw %0,%%fs" :: "r" (fs32_sel) ); #endif #ifdef DS_sig DS_sig(sigcontext) = ds64_sel; @@ -915,7 +916,7 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex * * Build a sigcontext from the register values. */ -static void restore_context( const struct xcontext *xcontext, ucontext_t *sigcontext ) +static void restore_context( const struct xcontext *xcontext, ucontext_t *sigcontext, BOOL restore_fs ) { const CONTEXT *context = &xcontext->c; XSTATE *xs; @@ -930,7 +931,7 @@ static void restore_context( const struct xcontext *xcontext, ucontext_t *sigcon if (FPU_sig(sigcontext)) *FPU_sig(sigcontext) = context->FltSave; if ((cpu_info.ProcessorFeatureBits & CPU_FEATURE_AVX) && (xs = XState_sig(FPU_sig(sigcontext)))) xs->CompactionMask = xcontext->host_compaction_mask; - leave_handler( sigcontext ); + leave_handler( sigcontext, restore_fs ); } @@ -1394,7 +1395,7 @@ NTSTATUS get_thread_wow64_context( HANDLE handle, void *ctx, ULONG size ) /*********************************************************************** * setup_raise_exception */ -static void setup_raise_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec, struct xcontext *xcontext ) +static void setup_raise_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec, struct xcontext *xcontext, BOOL restore_fs ) { void *stack_ptr = (void *)(RSP_sig(sigcontext) & ~15); CONTEXT *context = &xcontext->c; @@ -1423,7 +1424,7 @@ static void setup_raise_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec status = send_debug_event( rec, context, TRUE ); if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) { - restore_context( xcontext, sigcontext ); + restore_context( xcontext, sigcontext, restore_fs ); return; } @@ -1459,7 +1460,7 @@ static void setup_raise_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec RSP_sig(sigcontext) = (ULONG_PTR)stack; /* clear single-step, direction, and align check flag */ EFL_sig(sigcontext) &= ~(0x100|0x400|0x40000); - leave_handler( sigcontext ); + leave_handler( sigcontext, restore_fs ); } @@ -1470,13 +1471,13 @@ static void setup_raise_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec * sigcontext so that the return from the signal handler will call * the raise function. */ -static void setup_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec ) +static void setup_exception( ucontext_t *sigcontext, EXCEPTION_RECORD *rec, BOOL restore_fs ) { struct xcontext context; rec->ExceptionAddress = (void *)RIP_sig(sigcontext); save_context( &context, sigcontext ); - setup_raise_exception( sigcontext, rec, &context ); + setup_raise_exception( sigcontext, rec, &context, restore_fs ); } @@ -1789,7 +1790,7 @@ static inline DWORD is_privileged_instr( CONTEXT *context ) * * Handle an interrupt. */ -static inline BOOL handle_interrupt( ucontext_t *sigcontext, EXCEPTION_RECORD *rec, struct xcontext *xcontext ) +static inline BOOL handle_interrupt( ucontext_t *sigcontext, EXCEPTION_RECORD *rec, struct xcontext *xcontext, BOOL restore_fs ) { CONTEXT *context = &xcontext->c; @@ -1802,7 +1803,7 @@ static inline BOOL handle_interrupt( ucontext_t *sigcontext, EXCEPTION_RECORD *r rec->NumberParameters = 1; rec->ExceptionInformation[0] = context->Rcx; NtRaiseException( rec, context, FALSE ); - leave_handler( sigcontext ); + leave_handler( sigcontext, restore_fs ); return TRUE; case 0x2c: rec->ExceptionCode = STATUS_ASSERTION_FAILURE; @@ -1817,7 +1818,7 @@ static inline BOOL handle_interrupt( ucontext_t *sigcontext, EXCEPTION_RECORD *r case 4: /* BREAKPOINT_UNLOAD_SYMBOLS */ case 5: /* BREAKPOINT_COMMAND_STRING (>= Win2003) */ RIP_sig(sigcontext) += 3; - leave_handler( sigcontext ); + leave_handler( sigcontext, restore_fs ); return TRUE; } } @@ -1830,7 +1831,7 @@ static inline BOOL handle_interrupt( ucontext_t *sigcontext, EXCEPTION_RECORD *r default: return FALSE; } - setup_raise_exception( sigcontext, rec, xcontext ); + setup_raise_exception( sigcontext, rec, xcontext, restore_fs ); return TRUE; } @@ -1927,9 +1928,10 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext ) */ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { + BOOL restore_fs = FALSE; EXCEPTION_RECORD rec = { 0 }; struct xcontext context; - ucontext_t *ucontext = init_handler( sigcontext ); + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); rec.ExceptionAddress = (void *)RIP_sig(ucontext); save_context( &context, ucontext ); @@ -1953,7 +1955,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { WORD err = ERROR_sig(ucontext); if (!err && (rec.ExceptionCode = is_privileged_instr( &context.c ))) break; - if ((err & 7) == 2 && handle_interrupt( ucontext, &rec, &context )) return; + if ((err & 7) == 2 && handle_interrupt( ucontext, &rec, &context, restore_fs )) return; rec.ExceptionCode = EXCEPTION_ACCESS_VIOLATION; rec.NumberParameters = 2; rec.ExceptionInformation[0] = 0; @@ -1968,7 +1970,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) (void *)RSP_sig(ucontext) ); if (!rec.ExceptionCode) { - leave_handler( ucontext ); + leave_handler( ucontext, restore_fs ); return; } if (rec.ExceptionCode == EXCEPTION_ACCESS_VIOLATION && @@ -1985,7 +1987,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) if (EFL_sig(ucontext) & 0x00040000) { EFL_sig(ucontext) &= ~0x00040000; /* reset AC flag */ - leave_handler( ucontext ); + leave_handler( ucontext, restore_fs ); return; } rec.ExceptionCode = EXCEPTION_DATATYPE_MISALIGNMENT; @@ -2003,7 +2005,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) break; } if (handle_syscall_fault( ucontext, &rec, &context.c )) return; - setup_raise_exception( ucontext, &rec, &context ); + setup_raise_exception( ucontext, &rec, &context, restore_fs ); } @@ -2014,9 +2016,10 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { + BOOL restore_fs = FALSE; EXCEPTION_RECORD rec = { 0 }; struct xcontext context; - ucontext_t *ucontext = init_handler( sigcontext ); + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); if (handle_syscall_trap( ucontext )) return; @@ -2037,7 +2040,7 @@ static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) rec.ExceptionInformation[0] = 0; break; } - setup_raise_exception( ucontext, &rec, &context ); + setup_raise_exception( ucontext, &rec, &context, restore_fs ); } @@ -2048,8 +2051,9 @@ static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { + BOOL restore_fs = FALSE; EXCEPTION_RECORD rec = { 0 }; - ucontext_t *ucontext = init_handler( sigcontext ); + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); switch (siginfo->si_code) { @@ -2090,7 +2094,7 @@ static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) rec.ExceptionInformation[1] = FPU_sig(ucontext) ? FPU_sig(ucontext)->MxCsr : 0; if (CS_sig(ucontext) != cs64_sel) rec.ExceptionCode = STATUS_FLOAT_MULTIPLE_TRAPS; } - setup_exception( ucontext, &rec ); + setup_exception( ucontext, &rec, restore_fs ); } @@ -2101,7 +2105,8 @@ static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { - ucontext_t *ucontext = init_handler( sigcontext ); + BOOL restore_fs = FALSE; + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); HANDLE handle; if (p__wine_ctrl_routine) @@ -2110,7 +2115,7 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) p__wine_ctrl_routine, 0 /* CTRL_C_EVENT */, 0, 0, 0, 0, NULL )) NtClose( handle ); } - leave_handler( ucontext ); + leave_handler( ucontext, restore_fs ); } @@ -2121,10 +2126,11 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { - ucontext_t *ucontext = init_handler( sigcontext ); + BOOL restore_fs = FALSE; + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); EXCEPTION_RECORD rec = { EXCEPTION_WINE_ASSERTION, EH_NONCONTINUABLE }; - setup_exception( ucontext, &rec ); + setup_exception( ucontext, &rec, restore_fs ); } @@ -2135,7 +2141,8 @@ static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { - ucontext_t *ucontext = init_handler( sigcontext ); + BOOL restore_fs = FALSE; + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); if (!is_inside_syscall( ucontext )) user_mode_abort_thread( 0, amd64_thread_data()->syscall_frame ); abort_thread( 0 ); @@ -2149,7 +2156,8 @@ 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 ); + BOOL restore_fs = FALSE; + ucontext_t *ucontext = init_handler( sigcontext, &restore_fs ); struct xcontext context; if (is_inside_syscall( ucontext )) @@ -2161,13 +2169,13 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) NtGetContextThread( GetCurrentThread(), &context.c ); wait_suspend( &context.c ); NtSetContextThread( GetCurrentThread(), &context.c ); - leave_handler( sigcontext ); + leave_handler( sigcontext, restore_fs ); } else { save_context( &context, ucontext ); wait_suspend( &context.c ); - restore_context( &context, ucontext ); + restore_context( &context, ucontext, restore_fs ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4683
When this happens, it can be received within the small range of instructions where %fs has been restored but we have not yet switched to the syscall stack, or the other way around in the return path.
Is there a reason that we can't do these things in the right order then? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4683#note_55843
On Tue Dec 12 22:44:57 2023 +0000, Alexandre Julliard wrote:
When this happens, it can be received within the small range of instructions where %fs has been restored but we have not yet switched to the syscall stack, or the other way around in the return path. Is there a reason that we can't do these things in the right order then? It might not be possible to restore %fs *after* switching to the user stack as some of the return paths use `iret` to do that?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4683#note_55860
On Tue Dec 12 22:44:57 2023 +0000, Rémi Bernon wrote:
It might not be possible to restore %fs *after* switching to the user stack as some of the return paths use `iret` to do that? The iret path moves %rsp above the syscall frame so it should still do the right thing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4683#note_55879
participants (2)
-
Alexandre Julliard (＠julliard) -
Rémi Bernon