[PATCH v4 0/2] MR7064: Draft: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[William Horvath (＠whrvth)]

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

[PATCH v4 1/2] ntdll: Allow get_current_teb assembly to be used on all platforms.

[William Horvath]

From: William Horvath

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

[PATCH v4 2/2] ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[William Horvath]

From: William Horvath

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)

[PATCH v4 2/2] ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Marvin]

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

[William Horvath (＠whrvth)]

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.

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[William Horvath (＠whrvth)]

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.

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Jinoh Kang (＠iamahuman)]

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).

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Jinoh Kang (＠iamahuman)]

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 );
```

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Jinoh Kang (＠iamahuman)]

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;
```

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Jinoh Kang (＠iamahuman)]

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)

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Jinoh Kang (＠iamahuman)]

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.

[PATCH v4 0/2] MR7064: ntdll: Check for invalid %gs value in 32-bit code on WoW64.

[Jinoh Kang (＠iamahuman)]

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?

[William Horvath (＠whrvth)]

Marking as draft again until I can sort out the problem occurring when the program is 64-bit PE.

[Carlos Rodriguez (＠mrdeathjr28)]

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