Re: [PATCH 3/4] ntdll: Add CFI expressions for __wine_syscall_dispatcher.

[Rémi Bernon]

On 2/9/22 17:00, Jinoh Kang wrote:
> 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
> >
> > 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.

Yes, I added this here only because there's the same thing after the 
stack change and call, right after we restore the stack pointer. The 
latter is needed I think, as I understand the CFI are valid from their 
location up to the next cfi_endproc, and we want to restore the PE 
unwinding info as soon as we're off the unix stack.

I guess then this should instead be done in the same way as glibc does 
for it to be correct, with cfi_endproc + cfi_startproc inserted on the 
boundaries where we're switching stacks.
> > -
> >               __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...

Yes, GDB isn't happy about the syscall frame being above the PE stack, 
it then believes they are on the same stack but that the syscall frame 
frame is inner. For some reason, it now manages to unwind in my local 
setup, though I may just have hacked out the heuristic in a local GDB 
build because it was annoying.

In any case if we manage to link the unix frames together it should then 
happily unwind the syscall frames up to the exit frame and skip all the 
PE ones. It is a bit unfortunate for debugging purposes, but there's not 
much to do about it, and this will fix this error.
> >                  /* 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).

Yeah, I didn't do it because I have no idea which registers are needed 
there.
> [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/li...
> [3] https://elixir.bootlin.com/glibc/glibc-2.35.9000/source/sysdeps/unix/sysv/li...

Thanks for the help!