Re: [PATCH] x86_64,entry: Fix RCX for traced syscalls

From: Andy Lutomirski
Date: Tue Nov 04 2014 - 18:44:11 EST


On Thu, Jun 26, 2014 at 12:08 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> The int_ret_from_sys_call and syscall tracing code disagrees with
> the sysret path as to the value of RCX.
>
> The Intel SDM, the AMD APM, and my laptop all agree that sysret
> returns with RCX == RIP. The syscall tracing code does not respect
> this property.
>
> For example, this program:
>
> int main()
> {
> extern const char syscall_rip[];
> unsigned long rcx = 1;
> unsigned long orig_rcx = rcx;
> asm ("mov $-1, %%eax\n\t"
> "syscall\n\t"
> "syscall_rip:"
> : "+c" (rcx) : : "r11");
> printf("syscall: RCX = %lX RIP = %lX orig RCX = %lx\n",
> rcx, (unsigned long)syscall_rip, orig_rcx);
> return 0;
> }
>
> prints:
> syscall: RCX = 400556 RIP = 400556 orig RCX = 1
>
> Running it under strace gives this instead:
> syscall: RCX = FFFFFFFFFFFFFFFF RIP = 400556 orig RCX = 1
>
> This changes FIXUP_TOP_OF_STACK to match sysret, causing the test to
> show RCX == RIP even under strace.
>
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> ---
> arch/x86/kernel/entry_64.S | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index b25ca96..6624e18 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -143,7 +143,8 @@ ENDPROC(native_usergs_sysret64)
> movq \tmp,RSP+\offset(%rsp)
> movq $__USER_DS,SS+\offset(%rsp)
> movq $__USER_CS,CS+\offset(%rsp)
> - movq $-1,RCX+\offset(%rsp)
> + movq RIP+\offset(%rsp),\tmp /* get rip */
> + movq \tmp,RCX+\offset(%rsp) /* copy it to rcx as sysret would do */
> movq R11+\offset(%rsp),\tmp /* get eflags */
> movq \tmp,EFLAGS+\offset(%rsp)
> .endm
> --
> 1.9.3
>

Hi all-

I think this got lost. No one acked it, but no one nacked it either.
Summary of arguments in favor of applying:

- It's arguably a bugfix.

- Processes shouldn't be able to detect that they're being traced.
There are probably any number of ways that tracing is visible, but
this fixes one of them.

- The assembly code is complex enough anyway without requiring people
to wonder why setting the saved RCX to -1 is a reasonable thing to do.

- The performance hit is probably negligible. It's a single
instruction, it only happens during a slow path, and it's unlikely to
cause a cache miss.

Summary of arguments against:

- It adds one instruction.

- The bug it fixes isn't really entirely obviously a bug in the first place.

I'm still in favor.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/