[PATCH v3 0/3] MR1152: ntdll: Fixes for Overwatch 2.
Overwatch 2 doesn't like the win32u `-syscall` conversion that happened a while back, because it hooks KiUserCallbackDispatcher. This MR fixes it up to match what Overwatch 2 expects. -- v3: ntdll: Pass KiUserCallbackDispatcher parameters on stack. ntdll: Add 5-byte nop to start of KiUserCallbackDispatcher. ntdll: Align stack pointer when calling KiUserCallbackDispatcher. https://gitlab.winehq.org/wine/wine/-/merge_requests/1152
From: Torge Matthies <tmatthies(a)codeweavers.com> Overwatch hooks KiUserCallbackDispatcher and expects the stack pointer to be aligned to a multiple of 16 bytes, instead of the usual 8-byte misalignment, otherwise it will crash on a misaligned movaps. Fix this by aligning the stack pointer when calling the dispatcher and again inside the dispatcher. Signed-off-by: Torge Matthies <openglfreak(a)googlemail.com> --- dlls/ntdll/signal_x86_64.c | 18 ++++++++++++------ dlls/ntdll/unix/signal_x86_64.c | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index b1ab4933b93..59dc20ed693 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -654,12 +654,7 @@ __ASM_GLOBAL_FUNC( KiUserApcDispatcher, "int3") -/******************************************************************* - * KiUserCallbackDispatcher (NTDLL.@) - * - * FIXME: not binary compatible - */ -void WINAPI KiUserCallbackDispatcher( ULONG id, void *args, ULONG len ) +void WINAPI user_callback_dispatcher( ULONG id, void *args, ULONG len ) { NTSTATUS status; @@ -678,6 +673,17 @@ void WINAPI KiUserCallbackDispatcher( ULONG id, void *args, ULONG len ) RtlRaiseStatus( status ); } +/******************************************************************* + * KiUserCallbackDispatcher (NTDLL.@) + * + * FIXME: not binary compatible + */ +__ASM_GLOBAL_FUNC( KiUserCallbackDispatcher, + "andq $0xFFFFFFFFFFFFFFF0, %rsp\n\t" + __ASM_SEH(".seh_endprologue\n\t") + "call " __ASM_NAME("user_callback_dispatcher") "\n\t" + "int3") + static ULONG64 get_int_reg( CONTEXT *context, int reg ) { diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 5787f1dc6f9..db01bc3a736 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -2410,7 +2410,7 @@ NTSTATUS WINAPI KeUserModeCallback( ULONG id, const void *args, ULONG len, void callback_frame.frame.fs = amd64_thread_data()->fs; callback_frame.frame.gs = ds64_sel; callback_frame.frame.ss = ds64_sel; - callback_frame.frame.rsp = (ULONG_PTR)args_data - 0x28; + callback_frame.frame.rsp = (ULONG_PTR)args_data - 0x30; callback_frame.frame.rip = (ULONG_PTR)pKiUserCallbackDispatcher; callback_frame.frame.eflags = 0x200; callback_frame.frame.restore_flags = CONTEXT_CONTROL | CONTEXT_INTEGER; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1152
From: Torge Matthies <tmatthies(a)codeweavers.com> Overwatch 2 hooks KiUserCallbackDispatcher by overwriting the first five bytes with a jump, and returning to just after the jump. Make sure there is a five-byte instruction for it to replace. Signed-off-by: Torge Matthies <openglfreak(a)googlemail.com> --- dlls/ntdll/signal_x86_64.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index 59dc20ed693..64d0cb1be57 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -679,6 +679,7 @@ void WINAPI user_callback_dispatcher( ULONG id, void *args, ULONG len ) * FIXME: not binary compatible */ __ASM_GLOBAL_FUNC( KiUserCallbackDispatcher, + ".byte 0x0f, 0x1f, 0x44, 0x00, 0x00\n\t" /* Overwatch 2 replaces the first 5 bytes with a jump */ "andq $0xFFFFFFFFFFFFFFF0, %rsp\n\t" __ASM_SEH(".seh_endprologue\n\t") "call " __ASM_NAME("user_callback_dispatcher") "\n\t" -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1152
From: Torge Matthies <tmatthies(a)codeweavers.com> In addition to in registers. Overwatch 2 hooks KiUserCallbackDispatcher and expects to be able to use all the caller-saved registers, but also expects the callback id to be in ecx. Signed-off-by: Torge Matthies <openglfreak(a)googlemail.com> --- dlls/ntdll/signal_x86_64.c | 3 +++ dlls/ntdll/unix/signal_x86_64.c | 10 +++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index 64d0cb1be57..daf44f85c8f 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -680,6 +680,9 @@ void WINAPI user_callback_dispatcher( ULONG id, void *args, ULONG len ) */ __ASM_GLOBAL_FUNC( KiUserCallbackDispatcher, ".byte 0x0f, 0x1f, 0x44, 0x00, 0x00\n\t" /* Overwatch 2 replaces the first 5 bytes with a jump */ + "movq 0x28(%rsp), %rdx\n\t" + "movl 0x30(%rsp), %ecx\n\t" + "movl 0x34(%rsp), %r8d\n\t" "andq $0xFFFFFFFFFFFFFFF0, %rsp\n\t" __ASM_SEH(".seh_endprologue\n\t") "call " __ASM_NAME("user_callback_dispatcher") "\n\t" diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index db01bc3a736..78ee6bd0b6d 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -2400,8 +2400,16 @@ NTSTATUS WINAPI KeUserModeCallback( ULONG id, const void *args, ULONG len, void { struct syscall_frame *frame = amd64_thread_data()->syscall_frame; void *args_data = (void *)((frame->rsp - len) & ~15); + struct { + void *args; + ULONG id; + ULONG len; + } *params = (void *)((ULONG_PTR)args_data - 0x18); memcpy( args_data, args, len ); + params->args = args_data; + params->id = id; + params->len = len; callback_frame.frame.rcx = id; callback_frame.frame.rdx = (ULONG_PTR)args; @@ -2410,7 +2418,7 @@ NTSTATUS WINAPI KeUserModeCallback( ULONG id, const void *args, ULONG len, void callback_frame.frame.fs = amd64_thread_data()->fs; callback_frame.frame.gs = ds64_sel; callback_frame.frame.ss = ds64_sel; - callback_frame.frame.rsp = (ULONG_PTR)args_data - 0x30; + callback_frame.frame.rsp = (ULONG_PTR)params - 0x28; callback_frame.frame.rip = (ULONG_PTR)pKiUserCallbackDispatcher; callback_frame.frame.eflags = 0x200; callback_frame.frame.restore_flags = CONTEXT_CONTROL | CONTEXT_INTEGER; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1152
Changing the offset of `params` from `callback_frame.frame.rsp` actually breaks Overwatch again, so it's either subtracting 8 here or subtracting an extra 8 when calculating the address for `params`.
Adding some tests would be a good idea. You can look at the existing KiUserExceptionDispatcher tests for an example. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1152#note_12262
On Wed Oct 26 04:02:15 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 10/25/22 22:20, Torge Matthies (@tmatthies) wrote:
On Tue Oct 25 21:11:03 2022 +0000, Zebediah Figura wrote:
I was going to comment that this isn't a valid SEH prologue, but I guess it doesn't matter, since we do the same thing in the other Ki* functions. Perhaps those deserve a comment, or perhaps it's just clear enough that they're special... Should I just remove it? We can't unwind / backtrace through here anyway, so this SEH directive is useless.
I don't think I can answer that without understanding why the SEH directives are there in the first place. Paul, since you wrote them, can you by chance explain? ``` The windows version can unwind properly. The way this is usually achieved is by putting unwind opcodes at offset 0 (ie as if you put the macros before the first instruction) that restore the registers, and then finally rsp and rip using a machine frame uwop. In wine, you'd probably want to restore the frame that exited the Windows world.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1152#note_22141
On Wed Oct 26 16:37:32 2022 +0000, Torge Matthies wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/1152/diffs?diff_id=15680&start_sha=99c0aaa27bde828747380a99938c7360c5762b08#253a5ed0bf568582a6c96df257e6e01fe7220931_2402_2402) user-side kernel entries are intentionally misaligned, so that assembly stubs can directly call into a C handler. You can observe this in wine's KiUserExceptionDispatcher too. They also have the shadow space unused so that the C function doesn't clobber the things it receives pointers to as params.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1152#note_22142
This merge request was closed by Alexandre Julliard. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1152
Likely superseded by 0690851c9b52bbcd40b2e2e20a51ea555debe940. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1152#note_54530
participants (3)
-
Alexandre Julliard (＠julliard) -
novae.harpist06 (＠novae.harpist06)
-
Torge Matthies
-
Torge Matthies (＠tmatthies)