[PATCH 3/4] ntdll: Add CFI expressions for __wine_syscall_dispatcher.
Jin-oh Kang
jinoh.kang.kr at gmail.com
Wed Feb 9 18:58:22 CST 2022
On Thu, Feb 10, 2022, 2:54 AM Rémi Bernon <rbernon at codeweavers.com> wrote:
> 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 <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.
> >
>
> 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 think what we want to do here is to mark the end of the call stack with
".cfi_undefined %eip". If we do want to return to the PE side, we may as
well restore all the registers; a partial (not full) unwind would just make
debugging things harder.
> 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.
>
Sounds great.
Also, please keep in mind that glibc places its CFI pivot points after SP
switch only because the old stack is no longer valid (clone) or needed
(setcontext); as long as the CFA expression resolves to the same address
value and the current frame is valid, it's ok to leave the CFI state
unchanged.
> >> + __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.
>
For debugging purposes I think the user can find some way to inject dynamic
unwind info, so that shouldn't be a concern.
> >> /* 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.
>
I suppose we can start with SP and LR.
> > [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
> >
>
> Thanks for the help!
> --
> Rémi Bernon <rbernon at codeweavers.com>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220210/0e52e3d0/attachment.htm>
More information about the wine-devel
mailing list