[PATCH] ntdll: Set rcx on exit from syscall dispatcher on x64.
Jinoh Kang
jinoh.kang.kr at gmail.com
Wed Dec 1 10:15:48 CST 2021
Thanks a lot for taking time to read and consider my feedback!
Overall the patch looks good. I saw a few things that you might
have missed. Many of them are nitpicks, so feel free to ignore
them if you wish.
On 12/1/21 21:38, Paul Gofman wrote:
> Signed-off-by: Paul Gofman <pgofman at codeweavers.com>
> ---
> Supersedes 220540.
>
> Changes:
> - leave r11 alone for now;
> - also test NtSetContextThread with CONTEXT_INTEGER;
> - don't assume same stack pointer and non-volatile
> registers between test function calls in the test.
>
> dlls/ntdll/tests/exception.c | 76 +++++++++++++++++++++++++++++++++
> dlls/ntdll/unix/signal_x86_64.c | 11 +++--
> 2 files changed, 83 insertions(+), 4 deletions(-)
>
> diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c
> index 0362cb9f0a9..3019c1a40cb 100644
> --- a/dlls/ntdll/tests/exception.c
> +++ b/dlls/ntdll/tests/exception.c
> @@ -4917,6 +4917,81 @@ static void test_unwind_from_apc(void)
> ok(pass == 4, "Got unexpected pass %d.\n", pass);
> ok(test_unwind_apc_called, "Test user APC was not called.\n");
> }
> +
> +static void test_syscall_clobbered_regs(void)
> +{
> + struct regs
> + {
> + UINT64 rcx;
> + };
> + static const BYTE code[] =
> + {
> + 0x48, 0x8d, 0x05, 0x00, 0x10, 0x00, 0x00,
> + /* leaq 0x1000(%rip),%rax */
> + 0x48, 0x25, 0x00, 0xf0, 0xff, 0xff,
> + /* andq $~0xfff,%rax */
code_mem is already page-aligned, so &~0xfff should be unnecessary.
Even if this wasn't case, it might lead to the stack overlapping
the code if e.g. (rax & 0xfff) == 0xfff.
> + 0x48, 0x83, 0xe8, 0x08, /* subq $8,%rax */
> + 0x48, 0x89, 0x20, /* movq %rsp,0(%rax) */
> + 0x48, 0x89, 0xc4, /* movq %rax,%rsp */
I think "push %rsp" would make it clear that we're saving RSP. It's well known
that PUSH [ER]SP computes the source operand *before* decrementing the stack
pointer. Even if it's not immediately obvious to someone who reads the code,
they can still deduce from "POP RSP" at the epilogue that it's
saving/restoring the stack pointer.
All in all, my suggestion goes:
> 0x48, 0x8d, 0x25, 0xf9, 0x0f, 0x00, 0x00,
> /* leaq 0xff9(%rip),%rsp */
> 0x54, /* push %rsp */
> + 0x41, 0x50, /* push %r8 */
> + 0x53, 0x55, 0x57, 0x56, 0x41, 0x54, 0x41, 0x55, 0x41, 0x56, 0x41, 0x57,
> + /* push %rbx, %rbp, %rdi, %rsi, %r12, %r13, %r14, %r15 */
> + 0x41, 0xff, 0xd1, /* callq *r9 */
> + 0x41, 0x5f, 0x41, 0x5e, 0x41, 0x5d, 0x41, 0x5c, 0x5e, 0x5f, 0x5d, 0x5b,
> + /* pop %r15, %r14, %r13, %r12, %rsi, %rdi, %rbp, %rbx */
I think we should reload RSP beforehand, since we're assuming the stack pointer
isn't reliable at all after the system call.
> + 0x41, 0x58, /* pop %r8 */
> + 0x49, 0x89, 0x48, 0x00, /* mov %rcx,(%r8) */
> + 0x5c, /* pop %rsp */
(maybe swap the above two to keep the symmetry? I don't think it
matters much, though.)
> + 0xc3, /* ret */
> + };
> +
> + NTSTATUS (WINAPI *func)(void *arg1, void *arg2, struct regs *, void *call_addr);
> + NTSTATUS (WINAPI *pNtCancelTimer)(HANDLE, BOOLEAN *);
> + HMODULE hntdll = GetModuleHandleA("ntdll.dll");
> + struct regs regs;
> + CONTEXT context;
> + NTSTATUS status;
> +
> + pNtCancelTimer = (void *)GetProcAddress(hntdll, "NtCancelTimer");
> + ok(!!pNtCancelTimer, "NtCancelTimer not found.\n");
> + memcpy(code_mem, code, sizeof(code));
> + func = code_mem;
> + memset(®s, 0, sizeof(regs));
> + status = func((HANDLE)0xdeadbeef, NULL, ®s, pNtCancelTimer);
> + ok(status == STATUS_INVALID_HANDLE, "Got unexpected status %#x.\n", status);
> +
> + /* After the syscall instruction rcx contains the address of the instruction next after syscall. */
> + ok((BYTE *)regs.rcx > (BYTE *)pNtCancelTimer && (BYTE *)regs.rcx < (BYTE *)pNtCancelTimer + 0x20,
> + "Got unexpected rcx %s, pNtCancelTimer %p.\n", wine_dbgstr_longlong(regs.rcx), pNtCancelTimer);
> +
> + status = func((HANDLE)0xdeadbeef, (BOOLEAN *)0xdeadbeef, ®s, pNtCancelTimer);
> + ok(status == STATUS_ACCESS_VIOLATION, "Got unexpected status %#x.\n", status);
> + ok((BYTE *)regs.rcx > (BYTE *)pNtCancelTimer && (BYTE *)regs.rcx < (BYTE *)pNtCancelTimer + 0x20,
> + "Got unexpected rcx %s, pNtCancelTimer %p.\n", wine_dbgstr_longlong(regs.rcx), pNtCancelTimer);
> +
> + context.ContextFlags = CONTEXT_CONTROL;
> + status = func(GetCurrentThread(), &context, ®s, pNtGetContextThread);
> + ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status);
> + ok((BYTE *)regs.rcx > (BYTE *)pNtGetContextThread && (BYTE *)regs.rcx < (BYTE *)pNtGetContextThread + 0x20,
> + "Got unexpected rcx %s, pNtGetContextThread %p.\n", wine_dbgstr_longlong(regs.rcx), pNtGetContextThread);
> +
> + status = func(GetCurrentThread(), &context, ®s, pNtSetContextThread);
> + ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status);
> + ok((BYTE *)regs.rcx > (BYTE *)pNtGetContextThread && (BYTE *)regs.rcx < (BYTE *)pNtGetContextThread + 0x20,
> + "Got unexpected rcx %s, pNtGetContextThread %p.\n", wine_dbgstr_longlong(regs.rcx), pNtGetContextThread);
> +
> + context.ContextFlags = CONTEXT_INTEGER;
> + status = func(GetCurrentThread(), &context, ®s, pNtGetContextThread);
> + ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status);
> + ok((BYTE *)regs.rcx > (BYTE *)pNtGetContextThread && (BYTE *)regs.rcx < (BYTE *)pNtGetContextThread + 0x20,
> + "Got unexpected rcx %s, pNtGetContextThread %p.\n", wine_dbgstr_longlong(regs.rcx), pNtGetContextThread);
> +
> + status = func(GetCurrentThread(), &context, ®s, pNtSetContextThread);
> + ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status);
> + ok((BYTE *)regs.rcx > (BYTE *)pNtSetContextThread && (BYTE *)regs.rcx < (BYTE *)pNtSetContextThread + 0x20,
> + "Got unexpected rcx %s, pNtSetContextThread %p.\n", wine_dbgstr_longlong(regs.rcx), pNtSetContextThread);
> +
> +}
> #elif defined(__arm__)
>
> #define UNW_FLAG_NHANDLER 0
> @@ -10737,6 +10812,7 @@ START_TEST(exception)
> test_extended_context();
> test_copy_context();
> test_unwind_from_apc();
> + test_syscall_clobbered_regs();
>
> #elif defined(__aarch64__)
>
> diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c
> index 4310c1c6a2f..69625af1e24 100644
> --- a/dlls/ntdll/unix/signal_x86_64.c
> +++ b/dlls/ntdll/unix/signal_x86_64.c
> @@ -3236,18 +3236,21 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher,
> "testl $0x3,%edx\n\t" /* CONTEXT_CONTROL | CONTEXT_INTEGER */
> "jnz 1f\n\t"
> "movq 0x88(%rcx),%rsp\n\t"
> - "jmpq *0x70(%rcx)\n" /* frame->rip */
> + "movq 0x70(%rcx),%rcx\n\t" /* frame->rip */
> + "jmpq *%rcx\n\t"
> "1:\tleaq 0x70(%rcx),%rsp\n\t"
> "testl $0x2,%edx\n\t" /* CONTEXT_INTEGER */
> - "jz 1f\n\t"
> - "movq 0x00(%rcx),%rax\n\t"
> + "jnz 1f\n\t"
> + "movq (%rsp),%rcx\n\t" /* frame->rip */
CONTEXT_CONTROL means we should restore the CS segment register as well.
Since SYSRET cannot restore CS, Windows would have to use plain IRETQ
instead of SYSRET. In this case I suspect there should be no reason to
clobber RCX at all.
Note that this claim is unconfirmed; I might need some testing.
> + "iretq\n"
> + "1:\tmovq 0x00(%rcx),%rax\n\t"
> "movq 0x18(%rcx),%rdx\n\t"
> "movq 0x30(%rcx),%r8\n\t"
> "movq 0x38(%rcx),%r9\n\t"
> "movq 0x40(%rcx),%r10\n\t"
> "movq 0x48(%rcx),%r11\n\t"
> "movq 0x10(%rcx),%rcx\n"
> - "1:\tiretq\n"
> + "iretq\n"
> "5:\tmovl $0xc000000d,%edx\n\t" /* STATUS_INVALID_PARAMETER */
> "movq %rsp,%rcx\n"
> __ASM_NAME("__wine_syscall_dispatcher_return") ":\n\t"
>
--
Sincerely,
Jinoh Kang
More information about the wine-devel
mailing list