[PATCH 0/1] MR8899: ntdll: Fix crash in check_invalid_gsbase() on macOS.

[Brendan Shanks (＠bshanks)]

I saw a crash when Unix code would try to execute a NULL function pointer: while handling the SEGV `check_invalid_gsbase()` would try to deference `%rip` which is of course NULL.

This was only seen on macOS because the initial `if (cur_gsbase == (ULONG_PTR)teb)` check will never be equal, since there's no way [1] to find what GSBASE was when the signal was raised. macOS uses `%gs` for it's own thread-local storage, and we have to reset GSBASE to the Mac TSD in `init_handler()` before anything else runs.
This means that every page fault will result in reading from `%rip`, and `virtual_uninterrupted_read_memory()` must be used in case `%rip` isn't a valid address.

I also added some comments to explain the lack of a Mac codepath for getting/setting GSBASE.

With this commit and testing with the sample code in #57444, I found that on Intel Macs `check_invalid_gsbase()` is necessary and works correctly. Rosetta does not implement resetting GSBASE on a `pop %gs` though, so `check_invalid_gsbase()` isn't necessary there.

I do worry slightly about the overhead of calling `virtual_uninterrupted_read_memory()` for every page fault; if that ever needs to be reduced, skipping `check_invalid_gsbase()` on Rosetta would be a first step.


[1]: This is mostly but not completely true--when a user LDT is installed (i.e. wow64), macOS uses a larger "full" sigcontext that includes the value of GSBASE. But Rosetta doesn't implement this, so it's only useful on Intel Macs. And if we wanted to use it on Intel Macs, we would need to always install a user LDT.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8899

[PATCH 1/1] ntdll: Fix crash in check_invalid_gsbase() on macOS.

[Brendan Shanks]

From: Brendan Shanks <bshanks(a)codeweavers.com>

Jumping to 0x0 or a similarly invalid address would result in a
segfault in check_invalid_gsbase().
---
 dlls/ntdll/unix/signal_x86_64.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c
index e2585f57d31..b64d9ac648c 100644
--- a/dlls/ntdll/unix/signal_x86_64.c
+++ b/dlls/ntdll/unix/signal_x86_64.c
@@ -2051,8 +2051,8 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext, siginfo_t *siginfo )
  */
 static inline BOOL check_invalid_gsbase( ucontext_t *ucontext )
 {
-    unsigned int prefix_count = 0;
-    const BYTE *instr = (const BYTE *)RIP_sig( ucontext );
+    BYTE instr[16];
+    unsigned int i, len, prefix_count = 0;
     TEB *teb = NtCurrentTeb();
     ULONG_PTR cur_gsbase = 0;
 
@@ -2067,13 +2067,18 @@ static inline BOOL check_invalid_gsbase( ucontext_t *ucontext )
     amd64_get_gsbase( &cur_gsbase );
 #elif defined(__NetBSD__)
     sysarch( X86_64_GET_GSBASE, &cur_gsbase );
+#elif defined(__APPLE__)
+    /* init_handler() has already reset GSBASE, we can't determine what it was before the signal */
 #endif
 
     if (cur_gsbase == (ULONG_PTR)teb) return FALSE;
 
-    for (;;)
+    len = virtual_uninterrupted_read_memory( (BYTE *)RIP_sig(ucontext), instr, sizeof(instr) );
+    if (!len) return FALSE;
+
+    for (i = 0; i < len; i++)
     {
-        switch (*instr)
+        switch (instr[i])
         {
         /* instruction prefixes */
         case 0x2e:  /* %cs: */
@@ -2103,7 +2108,6 @@ static inline BOOL check_invalid_gsbase( ucontext_t *ucontext )
         case 0xf2:  /* repne */
         case 0xf3:  /* repe */
             if (++prefix_count >= 15) return FALSE;
-            instr++;
             continue;
         case 0x65:  /* %gs: */
             break;
@@ -2121,6 +2125,8 @@ static inline BOOL check_invalid_gsbase( ucontext_t *ucontext )
     amd64_set_gsbase( teb );
 #elif defined(__NetBSD__)
     sysarch( X86_64_SET_GSBASE, &teb );
+#elif defined(__APPLE__)
+    /* leave_handler() will set GSBASE to the TEB */
 #endif
     return TRUE;
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8899