[PATCH 0/3] MR11744: ntdll: Avoid exposing ARM64 syscall dispatcher to the client side.
Fixes regression from d3b41a854a8b45c0c4fac293bc57df8fe75245c4. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11744
From: Jacek Caban <jacek@codeweavers.com> --- dlls/ntdll/unix/signal_arm64.c | 11 ++++------- dlls/ntdll/unix/unix_private.h | 8 +------- dlls/ntdll/unix/virtual.c | 12 ++++++++++++ 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm64.c b/dlls/ntdll/unix/signal_arm64.c index a1d35cd5a44..9750ec55cc4 100644 --- a/dlls/ntdll/unix/signal_arm64.c +++ b/dlls/ntdll/unix/signal_arm64.c @@ -424,11 +424,8 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context ) frame->sp = context->Sp; frame->pc = context->Pc; frame->cpsr = context->Cpsr; - if (is_arm64ec()) - { - if (!is_ec_code( frame->pc )) flags |= RESTORE_FLAGS_EMULATION; - else frame->restore_flags &= ~RESTORE_FLAGS_EMULATION; - } + if (is_emulated_code( frame->pc )) flags |= RESTORE_FLAGS_EMULATION; + else frame->restore_flags &= ~RESTORE_FLAGS_EMULATION; } if (flags & CONTEXT_FLOATING_POINT) { @@ -1403,7 +1400,7 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *_sigcontext ) save_context( &context, sigcontext ); context.ContextFlags |= CONTEXT_EXCEPTION_REPORTING; wait_suspend( &context ); - if (is_arm64ec() && !is_ec_code( context.Pc )) + if (is_emulated_code( context.Pc )) { CONTEXT *user_context = (CONTEXT *)((context.Sp - sizeof(CONTEXT)) & ~15); @@ -1433,7 +1430,7 @@ static void usr2_handler( int signal, siginfo_t *siginfo, void *_sigcontext ) if (!is_inside_syscall( data, SP_sig(sigcontext) )) return; if (!frame) return; - if (is_arm64ec() && !is_ec_code( frame->pc )) + if (is_emulated_code( frame->pc )) { CONTEXT *user_context = (CONTEXT *)((frame->sp - sizeof(CONTEXT)) & ~15); diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index c8ca97d21a0..9a4db8e5d76 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -335,6 +335,7 @@ extern void virtual_fill_image_information( const struct pe_image_info *pe_info, SECTION_IMAGE_INFORMATION *info ); extern void *get_builtin_so_handle( void *module ); extern NTSTATUS set_builtin_unixlib_name( void *module, const char *name ); +extern BOOL is_emulated_code( ULONG_PTR ptr ); extern NTSTATUS get_thread_ldt_entry( HANDLE handle, THREAD_DESCRIPTOR_INFORMATION *info, ULONG len ); extern void *get_native_context( CONTEXT *context ); @@ -478,13 +479,6 @@ static inline BOOL is_inside_syscall( struct thread_data *data, ULONG_PTR sp ) (char *)sp <= (char *)get_syscall_frame( data )); } -static inline BOOL is_ec_code( ULONG_PTR ptr ) -{ - const UINT64 *map = (const UINT64 *)peb->EcCodeBitMap; - ULONG_PTR page = ptr / page_size; - return (map[page / 64] >> (page & 63)) & 1; -} - static inline CLIENT_ID make_client_id( ULONG pid, ULONG tid ) { CLIENT_ID id = { .UniqueProcess = ULongToHandle(pid), .UniqueThread = ULongToHandle(tid) }; diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index d6038c4dcb6..356b456bc29 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -2797,6 +2797,18 @@ static void *get_host_addr_space_limit(void) #ifdef __aarch64__ +/*********************************************************************** + * is_emulated_code + */ +BOOL is_emulated_code( ULONG_PTR ptr ) +{ + const UINT64 *map = (const UINT64 *)peb->EcCodeBitMap; + ULONG_PTR page = ptr / page_size; + if (!is_arm64ec() || ptr >= (ULONG_PTR)user_space_limit) return FALSE; + return !((map[page / 64] >> (page & 63)) & 1); +} + + /*********************************************************************** * alloc_arm64ec_map */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11744
From: Jacek Caban <jacek@codeweavers.com> --- dlls/ntdll/unix/signal_arm64.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm64.c b/dlls/ntdll/unix/signal_arm64.c index 9750ec55cc4..f1c196a117c 100644 --- a/dlls/ntdll/unix/signal_arm64.c +++ b/dlls/ntdll/unix/signal_arm64.c @@ -1660,15 +1660,15 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "stp x24, x25, [x10, #0xc0]\n\t" "stp x26, x27, [x10, #0xd0]\n\t" "stp x28, x29, [x10, #0xe0]\n\t" - "mov x19, sp\n\t" - "stp x9, x19, [x10, #0xf0]\n\t" - "mrs x9, NZCV\n\t" - "stp x30, x9, [x10, #0x100]\n\t" + "mov x11, sp\n\t" + "stp x9, x11, [x10, #0xf0]\n\t" + "mrs x12, NZCV\n\t" + "stp x30, x12, [x10, #0x100]\n\t" "str w8, [x10, #0x120]\n\t" - "mrs x9, FPCR\n\t" - "str w9, [x10, #0x128]\n\t" - "mrs x9, FPSR\n\t" - "str w9, [x10, #0x12c]\n\t" + "mrs x12, FPCR\n\t" + "str w12, [x10, #0x128]\n\t" + "mrs x12, FPSR\n\t" + "str w12, [x10, #0x12c]\n\t" "stp q0, q1, [x10, #0x130]\n\t" "stp q2, q3, [x10, #0x150]\n\t" "stp q4, q5, [x10, #0x170]\n\t" @@ -1685,11 +1685,10 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "stp q26, q27, [x10, #0x2d0]\n\t" "stp q28, q29, [x10, #0x2f0]\n\t" "stp q30, q31, [x10, #0x310]\n\t" - "mov x22, x10\n\t" /* switch to kernel stack */ "mov sp, x10\n\t" /* we're now on the kernel stack, stitch unwind info with previous frame */ - __ASM_CFI_CFA_IS_AT2(x22, 0x98, 0x02) /* frame->syscall_cfa */ + __ASM_CFI_CFA_IS_AT2(sp, 0x98, 0x02) /* frame->syscall_cfa */ __ASM_CFI(".cfi_offset 29, -0xc0\n\t") __ASM_CFI(".cfi_offset 30, -0xb8\n\t") __ASM_CFI(".cfi_offset 19, -0xb0\n\t") @@ -1702,6 +1701,8 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_CFI(".cfi_offset 26, -0x78\n\t") __ASM_CFI(".cfi_offset 27, -0x70\n\t") __ASM_CFI(".cfi_offset 28, -0x68\n\t") + "mov x22, sp\n\t" + __ASM_CFI_CFA_IS_AT2(x22, 0x98, 0x02) /* frame->syscall_cfa */ "and x20, x8, #0xfff\n\t" /* syscall number */ "ubfx x21, x8, #12, #2\n\t" /* syscall table number */ "ldr x16, [x18, #0x370]\n\t" /* thread_data->syscall_table */ @@ -1717,7 +1718,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "tbz x9, #3, 1f\n\t" "sub sp, sp, #8\n" "1:\tsub x9, x9, #8\n\t" - "ldr x10, [x19, x9]\n\t" + "ldr x10, [x11, x9]\n\t" "str x10, [sp, x9]\n\t" "cbnz x9, 1b\n" "2:\tldr x16, [x21]\n\t" /* table->ServiceTable */ @@ -1836,11 +1837,10 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, "stp x30, x9, [x10, #0xf0]\n\t" "mrs x9, NZCV\n\t" "stp x30, x9, [x10, #0x100]\n\t" - "mov x19, x10\n\t" /* switch to kernel stack */ "mov sp, x10\n\t" /* we're now on the kernel stack, stitch unwind info with previous frame */ - __ASM_CFI_CFA_IS_AT2(x19, 0x98, 0x02) /* frame->syscall_cfa */ + __ASM_CFI_CFA_IS_AT2(sp, 0x98, 0x02) /* frame->syscall_cfa */ __ASM_CFI(".cfi_offset 29, -0xc0\n\t") __ASM_CFI(".cfi_offset 30, -0xb8\n\t") __ASM_CFI(".cfi_offset 19, -0xb0\n\t") @@ -1853,6 +1853,8 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, __ASM_CFI(".cfi_offset 26, -0x78\n\t") __ASM_CFI(".cfi_offset 27, -0x70\n\t") __ASM_CFI(".cfi_offset 28, -0x68\n\t") + "mov x19, sp\n\t" + __ASM_CFI_CFA_IS_AT2(x19, 0x98, 0x02) /* frame->syscall_cfa */ "ldr x16, [x0, x1, lsl 3]\n\t" "mov x0, x2\n\t" /* args */ "blr x16\n\t" -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11744
From: Jacek Caban <jacek@codeweavers.com> When a signal occurs in the syscall dispatcher before it switches stacks, it is currently not considered to be inside a syscall, so a context containing the syscall dispatcher would be presented as a client context. Additionally, context modifications made during such a signal may be inconsistent. Avoid the problem by ensuring a consistent state in the SIGUSR1 handler. --- dlls/ntdll/unix/signal_arm64.c | 90 +++++++++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 11 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm64.c b/dlls/ntdll/unix/signal_arm64.c index f1c196a117c..caa80ae5961 100644 --- a/dlls/ntdll/unix/signal_arm64.c +++ b/dlls/ntdll/unix/signal_arm64.c @@ -263,21 +263,23 @@ static void syscall_frame_fixup_for_fastpath( struct syscall_frame *frame ) * * Set the FPU context from a sigcontext. */ -static void save_fpu( CONTEXT *context, const ucontext_t *sigcontext ) +static BOOL save_fpu( NEON128 vregs[32], ULONG *fpcr, ULONG *fpsr, const ucontext_t *sigcontext ) { #ifdef linux struct fpsimd_context *fp = get_fpsimd_context( sigcontext ); - if (!fp) return; - context->ContextFlags |= CONTEXT_FLOATING_POINT; - context->Fpcr = fp->fpcr; - context->Fpsr = fp->fpsr; - memcpy( context->V, fp->vregs, sizeof(context->V) ); + if (!fp) return FALSE; + *fpcr = fp->fpcr; + *fpsr = fp->fpsr; + memcpy( vregs, fp->vregs, 32 * sizeof(*vregs) ); + return TRUE; #elif defined(__APPLE__) - context->ContextFlags |= CONTEXT_FLOATING_POINT; - context->Fpcr = sigcontext->uc_mcontext->__ns.__fpcr; - context->Fpsr = sigcontext->uc_mcontext->__ns.__fpsr; - memcpy( context->V, sigcontext->uc_mcontext->__ns.__v, sizeof(context->V) ); + *fpcr = sigcontext->uc_mcontext->__ns.__fpcr; + *fpsr = sigcontext->uc_mcontext->__ns.__fpsr; + memcpy( vregs, sigcontext->uc_mcontext->__ns.__v, 32 * sizeof(*vregs) ); + return TRUE; +#else + return FALSE; #endif } @@ -320,7 +322,8 @@ static void save_context( CONTEXT *context, const ucontext_t *sigcontext ) context->Pc = PC_sig(sigcontext); /* Program Counter */ context->Cpsr = PSTATE_sig(sigcontext); /* Current State Register */ for (i = 0; i <= 28; i++) context->X[i] = REGn_sig( i, sigcontext ); - save_fpu( context, sigcontext ); + if (save_fpu( context->V, &context->Fpcr, &context->Fpsr, sigcontext )) + context->ContextFlags |= CONTEXT_FLOATING_POINT; } @@ -1361,6 +1364,31 @@ static void quit_handler( int signal, siginfo_t *siginfo, void *_sigcontext ) } +/*********************************************************************** + * save_syscall_entry_frame + * + * Save the syscall frame from the syscall dispatcher entry context. + */ +static void save_syscall_entry_frame( ucontext_t *sigcontext ) +{ + struct syscall_frame *frame = get_syscall_frame( get_thread_data() ); + unsigned int i; + + for (i = 18; i < 29; i++) frame->x[i] = REGn_sig( i, sigcontext ); + frame->fp = FP_sig( sigcontext ); + frame->lr = REGn_sig( 9, sigcontext ); + frame->sp = SP_sig(sigcontext); + frame->pc = LR_sig(sigcontext); + frame->cpsr = PSTATE_sig(sigcontext); + frame->restore_flags = 0; + frame->syscall_id = REGn_sig( 8, sigcontext ); + save_fpu( frame->v, &frame->fpcr, &frame->fpsr, sigcontext ); + + REGn_sig( 11, sigcontext ) = frame->sp; + SP_sig(sigcontext) = (ULONG_PTR)frame; +} + + /********************************************************************** * usr1_handler * @@ -1373,6 +1401,32 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *_sigcontext ) CHPE_V2_CPU_AREA_INFO *chpe; CONTEXT context; + extern const ULONG_PTR __wine_syscall_dispatcher_kernel_stack_ptr; + extern const ULONG_PTR __wine_syscall_dispatcher_user_stack; + extern const ULONG_PTR __wine_unix_call_dispatcher_kernel_stack_ptr; + extern const ULONG_PTR __wine_unix_call_dispatcher_user_stack; + + /* if we're in a syscall dispatcher, but not yet on the syscall stack, construct + * the frame now from the signal context. */ + if ((ULONG_PTR)__wine_syscall_dispatcher <= PC_sig(sigcontext) && + PC_sig(sigcontext) < __wine_syscall_dispatcher_kernel_stack_ptr) + { + save_syscall_entry_frame( sigcontext ); + PC_sig(sigcontext) = __wine_syscall_dispatcher_kernel_stack_ptr; + } + else if ((ULONG_PTR)__wine_unix_call_dispatcher <= PC_sig(sigcontext) && + PC_sig(sigcontext) < __wine_unix_call_dispatcher_kernel_stack_ptr) + { + save_syscall_entry_frame( sigcontext ); + PC_sig(sigcontext) = __wine_unix_call_dispatcher_kernel_stack_ptr; + } + else if (PC_sig(sigcontext) == __wine_syscall_dispatcher_user_stack || + PC_sig(sigcontext) == __wine_unix_call_dispatcher_user_stack) + { + struct syscall_frame *frame = get_syscall_frame( data ); + PC_sig(sigcontext) = frame->pc; + } + if (!data->teb) { server_select( NULL, 0, SELECT_INTERRUPTIBLE, 0, NULL, NULL ); @@ -1701,6 +1755,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_CFI(".cfi_offset 26, -0x78\n\t") __ASM_CFI(".cfi_offset 27, -0x70\n\t") __ASM_CFI(".cfi_offset 28, -0x68\n\t") + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_kernel_stack") ":\n\t" "mov x22, sp\n\t" __ASM_CFI_CFA_IS_AT2(x22, 0x98, 0x02) /* frame->syscall_cfa */ "and x20, x8, #0xfff\n\t" /* syscall number */ @@ -1779,6 +1834,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "ldp x30, x17, [sp, #0xf0]\n\t" /* switch to user stack */ "mov sp, x17\n\t" + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_user_stack") ":\n\t" "ret x16\n" __ASM_LOCAL_LABEL("trace_syscall") ":\n\t" @@ -1853,6 +1909,7 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, __ASM_CFI(".cfi_offset 26, -0x78\n\t") __ASM_CFI(".cfi_offset 27, -0x70\n\t") __ASM_CFI(".cfi_offset 28, -0x68\n\t") + __ASM_LOCAL_LABEL("__wine_unix_call_dispatcher_kernel_stack") ":\n\t" "mov x19, sp\n\t" __ASM_CFI_CFA_IS_AT2(x19, 0x98, 0x02) /* frame->syscall_cfa */ "ldr x16, [x0, x1, lsl 3]\n\t" @@ -1865,6 +1922,17 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, "ldp x16, x17, [sp, #0xf8]\n\t" /* switch to user stack */ "mov sp, x16\n\t" + __ASM_LOCAL_LABEL("__wine_unix_call_dispatcher_user_stack") ":\n\t" "ret x17" ) +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_syscall_dispatcher_kernel_stack_ptr"), + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_kernel_stack") ) +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_syscall_dispatcher_user_stack"), + __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_user_stack") ) + +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_unix_call_dispatcher_kernel_stack_ptr"), + __ASM_LOCAL_LABEL("__wine_unix_call_dispatcher_kernel_stack") ) +__ASM_GLOBAL_POINTER( __ASM_NAME("__wine_unix_call_dispatcher_user_stack"), + __ASM_LOCAL_LABEL("__wine_unix_call_dispatcher_user_stack") ) + #endif /* __aarch64__ */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11744
We're currently treating the syscall dispatcher entry code (before it switches the stack) like user-space code in SIGUSR1 handler, which leads to problems with the `is_ec_code` branch. If the dispatcher is out of user space limits, the `is_ec_code` call would crash. Otherwise, it would be treated as non-EC and a jump to an emulator would be setup. The root of the problem applies to other targets as well. For example, such dispatcher context would cause an error in MSVC debugger (which expects to be able to query memory attributes of memory under the current instruction pointer). It's more critical on ARM64, so I started with that. The first commit is not strictly needed: with dispatcher handling fixed, the `is_ec_code` call is no longer a problem. Still, it may be called on malformed user-provided context, so the additional check seems right. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11744#note_149678
participants (2)
-
Jacek Caban -
Jacek Caban (＠jacek)