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.
From: Torge Matthies tmatthies@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@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;
From: Torge Matthies tmatthies@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@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"
From: Torge Matthies tmatthies@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@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;
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.
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.
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.
This merge request was closed by Alexandre Julliard.
Likely superseded by 0690851c9b52bbcd40b2e2e20a51ea555debe940.
-
Alexandre Julliard (＠julliard) -
novae.harpist06 (＠novae.harpist06)
-
Torge Matthies
-
Torge Matthies (＠tmatthies)