[PATCH v4 0/2] MR7064: Draft: ntdll: Check for invalid %gs value in 32-bit code on WoW64.
Adapted from [check_invalid_gs](https://gitlab.winehq.org/wine/wine/-/blob/d7a7cae2e2d1ad34af6dbefcb06fbef2f...) in signal_i386.c. This prevents crashing when the %gs register is manipulated in 32-bit code on "new-style" WoW64. [test.c](https://bugs.winehq.org/attachment.cgi?id=77444) is a simple reproduction of the crash, courtesy of Fabian Maurer from the Bugzilla comments. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57444 -- v4: ntdll: Check for invalid %gs value in 32-bit code on WoW64. https://gitlab.winehq.org/wine/wine/-/merge_requests/7064
From: William Horvath <william(a)horvath.blog> And move mac_thread_gsbase around. --- dlls/ntdll/unix/signal_x86_64.c | 115 ++++++++++++++++---------------- 1 file changed, 57 insertions(+), 58 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index caa85249896..7667e96265b 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -462,14 +462,12 @@ static inline struct amd64_thread_data *amd64_thread_data(void) return (struct amd64_thread_data *)ntdll_get_thread_data()->cpu_data; } -#ifdef __linux__ static inline TEB *get_current_teb(void) { unsigned long rsp; __asm__( "movq %%rsp,%0" : "=r" (rsp) ); return (TEB *)(rsp & ~signal_stack_mask); } -#endif static BOOL is_inside_syscall( const ucontext_t *sigcontext ) { @@ -1825,6 +1823,63 @@ static inline DWORD is_privileged_instr( CONTEXT *context ) } +#ifdef __APPLE__ +/********************************************************************** + * mac_thread_gsbase + */ +static void *mac_thread_gsbase(void) +{ + struct thread_identifier_info tiinfo; + unsigned int info_count = THREAD_IDENTIFIER_INFO_COUNT; + static int gsbase_offset = -1; + + mach_port_t self = mach_thread_self(); + kern_return_t kr = thread_info(self, THREAD_IDENTIFIER_INFO, (thread_info_t) &tiinfo, &info_count); + mach_port_deallocate(mach_task_self(), self); + + if (kr == KERN_SUCCESS) return (void*)tiinfo.thread_handle; + + if (gsbase_offset < 0) + { + /* Search for the array of TLS slots within the pthread data structure. + That's what the macOS pthread implementation uses for gsbase. */ + const void* const sentinel1 = (const void*)0x2bffb6b4f11228ae; + const void* const sentinel2 = (const void*)0x0845a7ff6ab76707; + int rc; + pthread_key_t key; + const void** p = (const void**)pthread_self(); + int i; + + gsbase_offset = 0; + if ((rc = pthread_key_create(&key, NULL))) return NULL; + + pthread_setspecific(key, sentinel1); + + for (i = key + 1; i < 2000; i++) /* arbitrary limit */ + { + if (p[i] == sentinel1) + { + pthread_setspecific(key, sentinel2); + + if (p[i] == sentinel2) + { + gsbase_offset = (i - key) * sizeof(*p); + break; + } + + pthread_setspecific(key, sentinel1); + } + } + + pthread_key_delete(key); + } + + if (gsbase_offset) return (char*)pthread_self() + gsbase_offset; + return NULL; +} +#endif + + /*********************************************************************** * handle_interrupt * @@ -2420,62 +2475,6 @@ void signal_free_thread( TEB *teb ) } } -#ifdef __APPLE__ -/********************************************************************** - * mac_thread_gsbase - */ -static void *mac_thread_gsbase(void) -{ - struct thread_identifier_info tiinfo; - unsigned int info_count = THREAD_IDENTIFIER_INFO_COUNT; - static int gsbase_offset = -1; - - mach_port_t self = mach_thread_self(); - kern_return_t kr = thread_info(self, THREAD_IDENTIFIER_INFO, (thread_info_t) &tiinfo, &info_count); - mach_port_deallocate(mach_task_self(), self); - - if (kr == KERN_SUCCESS) return (void*)tiinfo.thread_handle; - - if (gsbase_offset < 0) - { - /* Search for the array of TLS slots within the pthread data structure. - That's what the macOS pthread implementation uses for gsbase. */ - const void* const sentinel1 = (const void*)0x2bffb6b4f11228ae; - const void* const sentinel2 = (const void*)0x0845a7ff6ab76707; - int rc; - pthread_key_t key; - const void** p = (const void**)pthread_self(); - int i; - - gsbase_offset = 0; - if ((rc = pthread_key_create(&key, NULL))) return NULL; - - pthread_setspecific(key, sentinel1); - - for (i = key + 1; i < 2000; i++) /* arbitrary limit */ - { - if (p[i] == sentinel1) - { - pthread_setspecific(key, sentinel2); - - if (p[i] == sentinel2) - { - gsbase_offset = (i - key) * sizeof(*p); - break; - } - - pthread_setspecific(key, sentinel1); - } - } - - pthread_key_delete(key); - } - - if (gsbase_offset) return (char*)pthread_self() + gsbase_offset; - return NULL; -} -#endif - /********************************************************************** * signal_init_process -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7064
From: William Horvath <william(a)horvath.blog> Adapted from check_invalid_gs in signal_i386.c. This prevents crashing when the %gs register is manipulated in 32-bit code on "new-style" WoW64. The SYSCALL_HAVE_WRFSGSBASE fast-path for getting/setting the segment registers on Linux is already checked in signal_init_process, and the other platforms' implementations follow existing conventions. Also note that we check in the TRAP_x86_PAGEFLT case because the error occurs when the invalid %gs value is actually used, i.e. by an internal call to NtCurrentTeb(). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57444 --- dlls/ntdll/unix/signal_x86_64.c | 60 +++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 7667e96265b..f2b75929148 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -2022,6 +2022,61 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext, siginfo_t *siginfo ) } +/*********************************************************************** + * check_invalid_gs + * + * Check for fault caused by invalid %gs value (some copy protection schemes mess with it). + * + */ +static inline BOOL check_invalid_gs( ucontext_t *sigcontext, CONTEXT *context ) +{ + const BYTE *instr = (const BYTE *)context->Rip; + TEB *teb = get_current_teb(); + WORD system_gs = ds64_sel; + ULONG_PTR cur_gs = 0; + +#ifdef __linux__ + if (syscall_flags & SYSCALL_HAVE_WRFSGSBASE) + __asm__ volatile ("rdgsbase %0" : "=r" (cur_gs)); + else + cur_gs = arch_prctl( ARCH_GET_GS, teb ); +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + amd64_get_gsbase( &cur_gs ); +#elif defined(__NetBSD__) + sysarch( X86_64_GET_GSBASE, &cur_gs ); +#elif defined(__APPLE__) + cur_gs = (ULONG_PTR)mac_thread_gsbase(); +#else +# error Please define getting %gs for your architecture +#endif + + if ((WORD)cur_gs == system_gs) return FALSE; + + TRACE( "%04hx/%04hx at %p, fixing up\n", (WORD)cur_gs, system_gs, instr ); + +#ifdef __linux__ + if (syscall_flags & SYSCALL_HAVE_WRFSGSBASE) + __asm__ volatile ("wrgsbase %0" :: "r" (teb)); + else + arch_prctl( ARCH_SET_GS, teb ); +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + amd64_set_gsbase( teb ); +#elif defined(__NetBSD__) + sysarch( X86_64_SET_GSBASE, &teb ); +#elif defined(__APPLE__) + __asm__ volatile ("movq %0,%%gs:%c1" :: "r" (teb->Tib.Self), + "n" (FIELD_OFFSET(TEB, Tib.Self))); + __asm__ volatile ("movq %0,%%gs:%c1" :: "r" (teb->ThreadLocalStoragePointer), + "n" (FIELD_OFFSET(TEB, ThreadLocalStoragePointer))); +#else +# error Please define setting %gs for your architecture +#endif + + GS_sig(sigcontext) = system_gs; + return TRUE; +} + + /********************************************************************** * segv_handler * @@ -2080,6 +2135,11 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) /* send EXCEPTION_EXECUTE_FAULT only if data execution prevention is enabled */ if (!(flags & MEM_EXECUTE_OPTION_DISABLE)) rec.ExceptionInformation[0] = EXCEPTION_READ_FAULT; } + if (is_wow64() && (CS_sig(ucontext) == cs64_sel) && check_invalid_gs( ucontext, &context.c )) + { + leave_handler( ucontext ); + return; + } break; case TRAP_x86_ALIGNFLT: /* Alignment check exception */ if (EFL_sig(ucontext) & 0x00040000) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7064
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150493 Your paranoid android. === debian11b (64 bit WoW report) === dinput: joystick8.c:5832: Test failed: input 1: WaitForSingleObject returned 0x102 joystick8.c:5833: Test failed: input 1: got 0 WM_INPUT messages joystick8.c:5836: Test failed: input 1: got dwType 0 joystick8.c:5837: Test failed: input 1: got header.dwSize 0 joystick8.c:5839: Test failed: input 1: got hDevice 0000000000000000 joystick8.c:5841: Test failed: input 1: got dwSizeHid 0 joystick8.c:5842: Test failed: input 1: got dwCount 0
On Sun Dec 22 23:22:21 2024 +0000, William Horvath wrote:
changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/7064/diffs?diff_id=150034&start_sha=9aa699aeffa626be6dac200ac777a6b39c9c5365#253a5ed0bf568582a6c96df257e6e01fe7220931_2040_2037) Thanks for the explanation and suggestion, this makes it much clearer. That's much simpler and should work just fine here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91076
On Sun Dec 22 23:30:51 2024 +0000, William Horvath wrote:
Some testing on macOS 13 showed that the check in `virtual_is_valid_code_address( instr, 1 )` leads to an early exit that breaks the fix, because the view->protect flags are missing `VPROT_SYSTEM`. I'm not sure what's causing this difference. For the minimal test program, simply removing that `virtual_is_valid_code_address` check allows it to work, but that doesn't seem like a robust solution. Fixed by checking for `CS_sig(sigcontext) == cs64_sel` instead as per Jinoh Kang's suggestion.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91079
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/signal_x86_64.c:
+ __asm__ volatile ("rdgsbase %0" : "=r" (cur_gs)); + else + cur_gs = arch_prctl( ARCH_GET_GS, teb ); +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + amd64_get_gsbase( &cur_gs ); +#elif defined(__NetBSD__) + sysarch( X86_64_GET_GSBASE, &cur_gs ); +#elif defined(__APPLE__) + cur_gs = (ULONG_PTR)mac_thread_gsbase(); +#else +# error Please define getting %gs for your architecture +#endif + + if ((WORD)cur_gs == system_gs) return FALSE; + + TRACE( "%04hx/%04hx at %p, fixing up\n", (WORD)cur_gs, system_gs, instr ); `signal_i386.c check_invalid_gs` had explicit check for GS: prefix. We should do something similar here.
See `is_privileged_instr` for a comprehensive list of prefixes on x86\_64. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91083
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/signal_x86_64.c:
/* send EXCEPTION_EXECUTE_FAULT only if data execution prevention is enabled */ if (!(flags & MEM_EXECUTE_OPTION_DISABLE)) rec.ExceptionInformation[0] = EXCEPTION_READ_FAULT; } + if (is_wow64() && (CS_sig(ucontext) == cs64_sel) && check_invalid_gs( ucontext, &context.c ))
```suggestion:-0+0 if (is_wow64() && CS_sig(ucontext) == cs64_sel && check_invalid_gs( ucontext, &context.c )) ``` Also, the cs64\_sel check belongs to check\_invalid\_gs especially when we have to check the code for GS prefix (among other 64-bit-exclusive prefixes like rex.W). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91084
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/signal_x86_64.c:
}
+/*********************************************************************** + * check_invalid_gs + * + * Check for fault caused by invalid %gs value (some copy protection schemes mess with it). + * + */ +static inline BOOL check_invalid_gs( ucontext_t *sigcontext, CONTEXT *context ) +{ + const BYTE *instr = (const BYTE *)context->Rip; + TEB *teb = get_current_teb(); + WORD system_gs = ds64_sel; + ULONG_PTR cur_gs = 0; In light of the GS/GSBASE confusion, I suggest you to rename this to `cur_gsbase`.
```suggestion:-0+0 ULONG_PTR cur_gsbase = 0; ``` (plus subsequent variable accesses) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91082
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/signal_x86_64.c:
+#ifdef __linux__ + if (syscall_flags & SYSCALL_HAVE_WRFSGSBASE) + __asm__ volatile ("rdgsbase %0" : "=r" (cur_gs)); + else + cur_gs = arch_prctl( ARCH_GET_GS, teb ); +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + amd64_get_gsbase( &cur_gs ); +#elif defined(__NetBSD__) + sysarch( X86_64_GET_GSBASE, &cur_gs ); +#elif defined(__APPLE__) + cur_gs = (ULONG_PTR)mac_thread_gsbase(); +#else +# error Please define getting %gs for your architecture +#endif + + if ((WORD)cur_gs == system_gs) return FALSE; You're confusing GSBASE (64-bit address registsr) with GS (16-bit segment register).
```suggestion:-0+0 if (cur_gs == (ULONG_PTR)teb) return FALSE; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91081
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/signal_x86_64.c:
+ * + * Check for fault caused by invalid %gs value (some copy protection schemes mess with it). + * + */ +static inline BOOL check_invalid_gs( ucontext_t *sigcontext, CONTEXT *context ) +{ + const BYTE *instr = (const BYTE *)context->Rip; + TEB *teb = get_current_teb(); + WORD system_gs = ds64_sel; + ULONG_PTR cur_gs = 0; + +#ifdef __linux__ + if (syscall_flags & SYSCALL_HAVE_WRFSGSBASE) + __asm__ volatile ("rdgsbase %0" : "=r" (cur_gs)); + else + cur_gs = arch_prctl( ARCH_GET_GS, teb ); ARCH\_GET\_GS yields value by writing to pointer. The return value is just error status.
```suggestion:-0+0 arch_prctl( ARCH_GET_GS, &cur_gs ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91080
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/signal_x86_64.c:
/* send EXCEPTION_EXECUTE_FAULT only if data execution prevention is enabled */ if (!(flags & MEM_EXECUTE_OPTION_DISABLE)) rec.ExceptionInformation[0] = EXCEPTION_READ_FAULT; } + if (is_wow64() && (CS_sig(ucontext) == cs64_sel) && check_invalid_gs( ucontext, &context.c ))
Pardon my mistake earlier. GS restoration is actually unncessary in system code. The condition should be `CS_sig(ucontext) == cs64_sel && virtual_is_valid_code_address( instr, 1 )`. Also, since you're probably on macOS, have you tried !6866? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91085
Marking as draft again until I can sort out the problem occurring when the program is 64-bit PE. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91088
On Mon Dec 23 03:24:51 2024 +0000, William Horvath wrote:
Marking as draft again until I can sort out the problem occurring when the program is 64-bit PE. In my case with your patch alice madness returns (steam), bloodrayne 1 and 2 back to work in new wow64
Thanks -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91091
participants (5)
-
Carlos Rodriguez (＠mrdeathjr28) -
Jinoh Kang (＠iamahuman)
-
Marvin -
William Horvath
-
William Horvath (＠whrvth)