[PATCH 3/4] ntdll: Add CFI expressions for __wine_syscall_dispatcher.
Jinoh Kang
jinoh.kang.kr at gmail.com
Wed Feb 9 10:00:11 CST 2022
On 2/8/22 04:05, Rémi Bernon wrote:
> Making sure stack pointer points to previous syscall / exit frame before
> entering a syscall, and restoring the PE frame information on return.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213
> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
> ---
> dlls/ntdll/unix/signal_i386.c | 9 +++++++++
> dlls/ntdll/unix/signal_x86_64.c | 9 +++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c
> index d98a3b1d4bb..2f6e2fd4153 100644
> --- a/dlls/ntdll/unix/signal_i386.c
> +++ b/dlls/ntdll/unix/signal_i386.c
> @@ -2492,6 +2492,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> "movl %esi,0x30(%ecx)\n\t"
> "movl %ebp,0x34(%ecx)\n\t"
> "leal 0x34(%ecx),%ebp\n\t"
> + __ASM_CFI(".cfi_def_cfa %ebp,0\n\t")
This changes the value of CFA. By definition, the actual value of CFA (not the CFA register) may never change within the context of a subroutine activation [1].
If we desire to switch CFA to a different frame anyway (with EIP overriden), we must end the current FDE with ".cfi_endproc" and start another FDE with ".cfi_startproc simple".
See [2] and [3] for how glibc achieves this.
> + __ASM_CFI(".cfi_rel_offset %eip,-0x2c\n\t")
This is the system call return address, which would be in a PE module.
I don't think this is very useful, since we will later switch to the exit frame anyway.
> + __ASM_CFI(".cfi_rel_offset %esp,-0x28\n\t")
This makes GDB unhappy: "previous frame inner to this frame (corrupt stack)?" [4].
This is caused by jumping from one stack (the thread stack) to another (the kernel/syscall stack). This usually never happens unless we're calling __morestack or handling a signal.
The solution would be to have .cfi_signal_frame at the start of FDE; not sure if it is worth it...
> "leal 4(%esp),%esi\n\t" /* first argument */
> "movl %eax,%ebx\n\t"
> "shrl $8,%ebx\n\t"
> @@ -2530,6 +2533,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> "3:\tfnsave 0x40(%ecx)\n\t"
> "fwait\n"
> "4:\tmovl %ecx,%esp\n\t"
> + __ASM_CFI(".cfi_def_cfa %ebp,0\n\t")
This is unnecessary sine we have already configured the CFA expression previously (unless we're going to start another FDE, of course).
Also, changing SP does not mean it is an unwind pivot by itself.
> + __ASM_CFI(".cfi_rel_offset %eip,0x34c\n\t") /* frame->unwind_rip */
> + __ASM_CFI(".cfi_rel_offset %esp,0x08\n\t") /* frame->prev_frame */
> "movl 0x1c(%esp),%edx\n\t" /* frame->eax */
> "andl $0xfff,%edx\n\t" /* syscall number */
> "cmpl 8(%ebx),%edx\n\t" /* table->ServiceLimit */
> @@ -2545,6 +2551,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> "rep; movsl\n\t"
> "call *(%eax,%edx,4)\n\t"
> "leal -0x34(%ebp),%esp\n"
> + __ASM_CFI(".cfi_def_cfa %ebp,0\n\t")
Ditto.
> + __ASM_CFI(".cfi_rel_offset %eip,-0x2c\n\t")
> + __ASM_CFI(".cfi_rel_offset %esp,-0x28\n\t")
> "5:\tmovl 0(%esp),%ecx\n\t" /* frame->syscall_flags + (frame->restore_flags << 16) */
> "testl $0x68 << 16,%ecx\n\t" /* CONTEXT_FLOATING_POINT | CONTEXT_EXTENDED_REGISTERS | CONTEXT_XSAVE */
> "jz 3f\n\t"
> diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c
> index f8cddd15569..ee2723cdb24 100644
> --- a/dlls/ntdll/unix/signal_x86_64.c
> +++ b/dlls/ntdll/unix/signal_x86_64.c
> @@ -3166,6 +3166,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> "movw %ss,0x90(%rcx)\n\t"
> "movw %gs,0x92(%rcx)\n\t"
> "movq %rbp,0x98(%rcx)\n\t"
> + __ASM_CFI(".cfi_def_cfa %rcx,0\n\t")
> + __ASM_CFI(".cfi_rel_offset %rip,0x70\n\t")
> + __ASM_CFI(".cfi_rel_offset %rsp,0x88\n\t")
Ditto for x86_64.
> /* Legends of Runeterra hooks the first system call return instruction, and
> * depends on us returning to it. Adjust the return address accordingly. */
> "subq $0xb,0x70(%rcx)\n\t"
> @@ -3206,6 +3209,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> #endif
> "leaq 0x28(%rsp),%rsi\n\t" /* first argument */
> "movq %rcx,%rsp\n\t"
> + __ASM_CFI(".cfi_def_cfa %rbp,0\n\t")
> + __ASM_CFI(".cfi_rel_offset %rip,0x20\n\t") /* frame->unwind_rip */
> + __ASM_CFI(".cfi_rel_offset %rsp,0x08\n\t") /* frame->prev_frame */
> "movq 0x00(%rcx),%rax\n\t"
> "movq 0x18(%rcx),%rdx\n\t"
> "movl %eax,%ebx\n\t"
> @@ -3231,6 +3237,9 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> "movq (%rbx),%r10\n\t" /* table->ServiceTable */
> "callq *(%r10,%rax,8)\n\t"
> "leaq -0x98(%rbp),%rcx\n"
> + __ASM_CFI(".cfi_def_cfa %rcx,0\n\t")
> + __ASM_CFI(".cfi_rel_offset %rip,0x70\n\t")
> + __ASM_CFI(".cfi_rel_offset %rsp,0x88\n\t")
> "2:\tmovl 0x94(%rcx),%edx\n\t" /* frame->restore_flags */
> #ifdef __linux__
> "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */
Also, it might be a good idea to apply this to the ARM side as well (although I'm not sure).
[1] DWARF Debugging Information Format Version 5, page 172, line 4 (§6.4 Call Frame Information), https://dwarfstd.org/doc/DWARF5.pdf ("(By definition, the CFA value does not change.)")
[2] https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/linux/x86_64/clone.S#L78
[3] https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/linux/x86_64/setcontext.S#L187
--
Sincerely,
Jinoh Kang
More information about the wine-devel
mailing list