Re: [PATCH] ntdll: Set rcx on exit from syscall dispatcher on x64.

[Jinoh Kang]

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
> 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(&regs, 0, sizeof(regs));
> - status = func((HANDLE)0xdeadbeef, NULL, &regs, 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, &regs, 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, &regs, 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, &regs, 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, &regs, 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, &regs, 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"