[PATCH v8 0/3] MR7064: ntdll: Check for invalid GS base in the 64-bit segv_handler.
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, such as in 32-bit code on "new-style" WoW64. "Exertus: Darkness Approaches" is a 32-bit game that triggers this crash (on WoW64), which can be downloaded from the Bugzilla page's attachments. For another example, [test.c](https://bugs.winehq.org/attachment.cgi?id=77444) can reproduce it, courtesy of Fabian Maurer from the Bugzilla comments. This will crash whether it's compiled as 32-bit or 64-bit, while it works fine on Windows. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57444 Also, excuse me if submitting this is against some sort of code-freeze etiquette, it wasn't my intention try to have it merged immediately. -- v8: ntdll/tests: Re-enable a previously crashing test. ntdll: Check for invalid gs_base in the 64-bit segv_handler. ntdll/tests: Add an exception test for accessing a modified %gs on x86_64. https://gitlab.winehq.org/wine/wine/-/merge_requests/7064
From: William Horvath <william(a)horvath.blog> --- dlls/ntdll/tests/exception.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index 9b5ca2ca508..7745a762cb5 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -2711,6 +2711,14 @@ static const struct exception { { 0xb8, 0x01, 0x00, 0x00, 0x00, /* mov $0x01, %eax */ 0xcd, 0x2d, 0xfa, 0xc3 }, /* int $0x2d; cli; ret */ 8, 0, STATUS_SUCCESS, 0 }, +#if 0 /* Disabled for the same reason as the #if 0 blocks above (gs_base zeroed) */ + { { 0x66, 0x0f, 0xa8, /* push %gs */ + 0x66, 0x0f, 0xa9, /* pop %gs */ + 0x65, 0x48, 0x8b, 0x04, 0x25, /* movq %gs:0x30,%rax (NtCurrentTeb) */ + 0x30, 0x00, 0x00, 0x00, + 0xc3 }, /* ret */ + 8, 0, STATUS_SUCCESS, 0 }, +#endif }; static int got_exception; -- 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. PE-side code can manipulate %gs and cause the next call to NtCurrentTeb to segfault, as the gs_base may be cleared with writes to %gs on x86_64 [1]. This would cause a recursive exception loop, as any PE-side code in the exception handling chain after the segv_handler would run into the same problem. So, catch this early, and manually repair the thread's gs_base with the pthread TEB from the Unix side. The 32-bit game "Alice: Madness Returns" is one example of this problem occurring in the real world, when running under WoW64. However, this is currently handled in Windows under both WoW64 and native 64-bit, so we should handle both architectures as well. [1]: https://bugs.winehq.org/show_bug.cgi?id=51152 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57444 --- dlls/ntdll/tests/exception.c | 2 - dlls/ntdll/unix/signal_x86_64.c | 101 ++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index 7745a762cb5..f50d47917e8 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -2711,14 +2711,12 @@ static const struct exception { { 0xb8, 0x01, 0x00, 0x00, 0x00, /* mov $0x01, %eax */ 0xcd, 0x2d, 0xfa, 0xc3 }, /* int $0x2d; cli; ret */ 8, 0, STATUS_SUCCESS, 0 }, -#if 0 /* Disabled for the same reason as the #if 0 blocks above (gs_base zeroed) */ { { 0x66, 0x0f, 0xa8, /* push %gs */ 0x66, 0x0f, 0xa9, /* pop %gs */ 0x65, 0x48, 0x8b, 0x04, 0x25, /* movq %gs:0x30,%rax (NtCurrentTeb) */ 0x30, 0x00, 0x00, 0x00, 0xc3 }, /* ret */ 8, 0, STATUS_SUCCESS, 0 }, -#endif }; static int got_exception; diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index caa85249896..46c70adc83b 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1967,6 +1967,102 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext, siginfo_t *siginfo ) } +/*********************************************************************** + * check_invalid_gsbase + * + * Check for fault caused by invalid %gs value (some copy protection schemes mess with it). + */ +static inline BOOL check_invalid_gsbase( ucontext_t *sigcontext, CONTEXT *context ) +{ + unsigned int prefix_count = 0; + const BYTE *instr = (const BYTE *)context->Rip; + TEB *teb = NtCurrentTeb(); + ULONG_PTR cur_gsbase = 0; + +#ifdef __linux__ + if (syscall_flags & SYSCALL_HAVE_WRFSGSBASE) + __asm__ volatile ("rdgsbase %0" : "=r" (cur_gsbase)); + else + arch_prctl( ARCH_GET_GS, &cur_gsbase ); +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + amd64_get_gsbase( &cur_gsbase ); +#elif defined(__NetBSD__) + sysarch( X86_64_GET_GSBASE, &cur_gsbase ); +#elif defined(__APPLE__) + cur_gsbase = (ULONG_PTR)teb->ThreadLocalStoragePointer; +#else +# error Please define getting %gs for your architecture +#endif + + if (cur_gsbase == (ULONG_PTR)teb) return FALSE; + + for (;;) + { + switch(*instr) + { + /* instruction prefixes */ + case 0x2e: /* %cs: */ + case 0x36: /* %ss: */ + case 0x3e: /* %ds: */ + case 0x26: /* %es: */ + case 0x40: /* rex */ + case 0x41: /* rex */ + case 0x42: /* rex */ + case 0x43: /* rex */ + case 0x44: /* rex */ + case 0x45: /* rex */ + case 0x46: /* rex */ + case 0x47: /* rex */ + case 0x48: /* rex */ + case 0x49: /* rex */ + case 0x4a: /* rex */ + case 0x4b: /* rex */ + case 0x4c: /* rex */ + case 0x4d: /* rex */ + case 0x4e: /* rex */ + case 0x4f: /* rex */ + case 0x64: /* %fs: */ + case 0x66: /* opcode size */ + case 0x67: /* addr size */ + case 0xf0: /* lock */ + case 0xf2: /* repne */ + case 0xf3: /* repe */ + if (++prefix_count >= 15) return FALSE; + instr++; + continue; + case 0x65: /* %gs: */ + GS_sig(sigcontext) = ds64_sel; + break; + default: + return FALSE; + } + break; /* %gs: */ + } + + TRACE( "gsbase %016lx teb %p at instr %p, fixing up\n", cur_gsbase, teb, 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 + + return TRUE; +} + + /********************************************************************** * segv_handler * @@ -2025,6 +2121,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 (CS_sig(ucontext) == cs64_sel && check_invalid_gsbase( 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
From: William Horvath <william(a)horvath.blog> See https://bugs.winehq.org/show_bug.cgi?id=51152 for the bug that led to commit 4e4847dd71a3c682356559a51705ccec93b2490e. We can re-enable the %gs case now, as that no longer causes a crash. --- dlls/ntdll/tests/exception.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index f50d47917e8..13ef9b693bf 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -2673,24 +2673,21 @@ static const struct exception /* It is observed that fs/gs base is reset on some CPUs when setting the segment value even to 0 (regardless of CPU spec - saying otherwise) and it is not currently - handled in Wine. + saying otherwise) and the fs base case + is not currently handled in Wine. Disable this part to avoid crashing the test. */ 0x8e, 0xe0, /* mov %eax,%fs */ - 0x8e, 0xe8, /* mov %eax,%gs */ #else 0x90, 0x90, /* nop */ - 0x90, 0x90, /* nop */ #endif + 0x8e, 0xe8, /* mov %eax,%gs */ 0xfa, /* cli */ 0x58, /* pop %rax */ -#if 0 0x8e, 0xe8, /* mov %eax,%gs */ 0x58, /* pop %rax */ +#if 0 0x8e, 0xe0, /* mov %eax,%fs */ #else - 0x58, /* pop %rax */ - 0x90, 0x90, /* nop */ 0x90, 0x90, /* nop */ #endif 0x58, /* pop %rax */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7064
v8: - added a test - once again changed to handle non-WoW64 exceptions after Windows testing showed that this is correct - corrected the commit description - added a commit to re-enable a previously broken (`#if 0`) test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92439
Given your participation in these adjacent incidences/issues: https://bugs.winehq.org/show_bug.cgi?id=51152 https://gitlab.winehq.org/wine/wine/-/merge_requests/5480#note_67836 do you mind taking a look, @gofman ? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92441
I didn't look in all the details or test that, but one thing that stands out to me in this patch is that analyzing instructions at fault address is very unfortunate. First, I suspect the way it is written now it may crash at handler if the under instruction pointer is not accessible (see is_privileged_instr() for the proper handling). That could be solved but analyzing the instructions here is also very unfortunate. It is a lot of complication and there are probably cases which are not covered by the current code, e. g., there could be some extra no-op prefixes which are frequently encountered in obfuscated code. Maybe a better way would be to check for the gsbase in the handler and if it is unexpected set it to the right value and continue from the faulted instruction? Do you have any real app depending on that or is it just to fix the test? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92478
On Wed Jan 22 17:46:41 2025 +0000, Paul Gofman wrote:
I didn't look in all the details or test that, but one thing that stands out to me in this patch is that analyzing instructions at fault address is very unfortunate. First, I suspect the way it is written now it may crash at handler if the under instruction pointer is not accessible (see is_privileged_instr() for the proper handling). That could be solved but analyzing the instructions here is also very unfortunate. It is a lot of complication and there are probably cases which are not covered by the current code, e. g., there could be some extra no-op prefixes which are frequently encountered in obfuscated code. Maybe a better way would be to check for the gsbase in the handler and if it is unexpected set it to the right value and continue from the faulted instruction? Do you have any real app depending on that or is it just to fix the test? That, and if it segfaults on a %gs-based instruction for reasons other than bad gsbase (multiple segment prefixes, or code pointer in nonexecutable page, for example), won't that give an infinite loop of the same segfault?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92480
On Wed Jan 22 17:46:41 2025 +0000, Alfred Agrell wrote:
That, and if it segfaults on a %gs-based instruction for reasons other than bad gsbase (multiple segment prefixes, or code pointer in nonexecutable page, for example), won't that give an infinite loop of the same segfault? The instruction analysis isn't required in order for this check to fix the tests and the games. It might as well be vestigial in this patch, actually - it's evolved from what I initially thought the problem was, and what I thought was required for a correct solution.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92481
On Wed Jan 22 17:50:27 2025 +0000, William Horvath wrote:
The instruction analysis isn't required in order for this check to fix the tests and the games. It might as well be vestigial in this patch, actually - it's evolved from what I initially thought the problem was, and what I thought was required for a correct solution. If it was simplified to just checking for a zeroed gsbase and replacing it with the Unix-side NtCurrentTeb, I don't see how that would introduce infinite loops that wouldn't have already been problems some other way? That was also a bit discussed here FWIW and has some ad-hoc diff which fixes those tests (and additional tests attached there): https://gitlab.winehq.org/wine/wine/-/merge_requests/5480#note_67898
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92482
On Wed Jan 22 17:50:32 2025 +0000, Paul Gofman wrote:
That was also a bit discussed here FWIW and has some ad-hoc diff which fixes those tests (and additional tests attached there): https://gitlab.winehq.org/wine/wine/-/merge_requests/5480#note_67898 Then, I am not immediately sure how that will interact with %gs specifics on MacOS, that needs some exploration or comment from someone versed in Macs.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92483
On Wed Jan 22 17:58:15 2025 +0000, Paul Gofman wrote:
Then, I am not immediately sure how that will interact with %gs specifics on MacOS, that needs some exploration or comment from someone versed in Macs. I think @bshanks was preparing something for correct %gs switch on Macs, is that the case? Maybe that should be considered before performing the specifics gsbase handling to fix this special case of %gs reset on the app side.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92484
On Wed Jan 22 18:01:22 2025 +0000, Paul Gofman wrote:
I think @bshanks was preparing something for correct %gs switch on Macs, is that the case? Maybe that should be considered before performing the specifics gsbase handling to fix this special case of %gs reset on the app side. I found that discussion recently, and yes, the basic principle of that ad-hoc diff is what I'm trying to do here as well.
WRT MacOS, this patch fixes the crash on WoW64 with a 32-bit program, but it still segfaults on pure 64-bit. I assume I'm missing some subtlety there. The MR you're thinking of was mentioned [here](https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_91085) (!6866), and I was meaning to try it, but I never got around to it with my cumbersome VM setup. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_92485
This fix somehow effect a game call 'Creature reaction inside the ship! 1.5' with native quartz.dll. it no longer shown anything if patch is applied. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_101008
On Wed Apr 16 05:20:01 2025 +0000, Chunhao Hung wrote:
This fix somehow effect a game call 'Creature reaction inside the ship! 1.5' with native quartz.dll. it no longer shown anything if patch is applied under wow64 enabled build. Here is screenshot, This should has a video render here.  Here is log with +seh,+unwind [stdout.gz](/uploads/0b5cde00b9610e9bab4780739b4ff840/stdout.gz)
This seems happened since I applied this patch, I just found out now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_101010
On Fri Apr 18 23:49:41 2025 +0000, Chunhao Hung wrote:
Here is screenshot, This should has a video render here. 
Here is log with +seh,+unwind [stdout.gz](/uploads/0b5cde00b9610e9bab4780739b4ff840/stdout.gz) This seems happened since I applied this patch, I just found out now. This is not effect when you are using builtin quartz.dll rather than native quartz.dll. I can't reproduce this issue. Some things I noticed:
1. `check_invalid_gs` is coming from `dlls/ntdll/unix/signal_i386.c`, not my patch. This also should be impossible to see with a proper new-WoW64 build of Wine, since the i386 ntdll.so is never used (or built at all). See here: ``` ❯ grep -irnw . -e ".*check_invalid_gs.*" grep: ./lib/wine/x86_64-unix/ntdll.so: binary file matches ❯ strings ./lib/wine/x86_64-unix/ntdll.so | grep check_invalid_gs check_invalid_gsbase check_invalid_gsbase check_invalid_gsbase ``` This leads me to believe your Wine build is broken in some way. `check_invalid_gs` should never be visible. 2. You're using a staging build of Wine. I tried this as well, but still couldn't reproduce the issue. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_101432
On Fri Apr 18 23:49:41 2025 +0000, William Horvath wrote:
I can't reproduce this issue. Some things I noticed: 1. `check_invalid_gs` is coming from `dlls/ntdll/unix/signal_i386.c`, not my patch. This also should be impossible to see with a proper new-WoW64 build of Wine, since the i386 ntdll.so is never used (or built at all). See here: ``` ❯ grep -irnw . -e ".*check_invalid_gs.*" grep: ./lib/wine/x86_64-unix/ntdll.so: binary file matches ❯ strings ./lib/wine/x86_64-unix/ntdll.so | grep check_invalid_gs check_invalid_gsbase check_invalid_gsbase check_invalid_gsbase ``` This leads me to believe your Wine build is broken in some way. `check_invalid_gs` should never be visible. 2. You're using a staging build of Wine. I tried this as well, but still couldn't reproduce the issue. thanks for point out, that is your patch yes, but it is the version 1. I did not updated when you updated. I will try out the latest one if the fix still working.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7064#note_101434
participants (6)
-
Alfred Agrell (＠Alcaro) -
Chunhao Hung (＠otakuxtom)
-
Paul Gofman (＠gofman)
-
William Horvath
-
William Horvath (＠whrvt)
-
William Horvath (＠whrvth)