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.
From: William Horvath william@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;
From: William Horvath william@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)
From: William Horvath william@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 */
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.
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 ?
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?
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?
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.
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
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.
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.
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.
-
Alfred Agrell (＠Alcaro) -
Paul Gofman (＠gofman)
-
William Horvath
-
William Horvath (＠whrvth)