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

[Andy Lutomirski]

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/