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

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

[Andy Lutomirski]

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@amacapital.net>
---
 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

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

[Andi Kleen]

> show RCX == RIP even under strace.

If you think it's really worth the extra instruction?

It's not wrong, but it's not clear if it's useful.

-Andi

> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  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
> 

-- 
ak@linux.intel.com -- Speaking for myself only.

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

[Andy Lutomirski]

On Thu, Jun 26, 2014 at 12:56 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> show RCX == RIP even under strace.
>
> If you think it's really worth the extra instruction?

Hard to say.  That extra instruction only happens on slow paths, so I
suspect the slowdown is negligible.  On the other hand, having syscall
show a blatant difference in behavior between traced and untraced
processes seems unfortunate.

>
> It's not wrong, but it's not clear if it's useful.

--Andy

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

[Andy Lutomirski]

On Thu, Jun 26, 2014 at 12:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jun 26, 2014 at 12:56 PM, Andi Kleen <andi@firstfloor.org> wrote:
>>> show RCX == RIP even under strace.
>>
>> If you think it's really worth the extra instruction?
>
> Hard to say.  That extra instruction only happens on slow paths, so I
> suspect the slowdown is negligible.  On the other hand, having syscall
> show a blatant difference in behavior between traced and untraced
> processes seems unfortunate.
>
>>
>> It's not wrong, but it's not clear if it's useful.

Also, if anyone ever wants to add some code to switch back from iret
to sysret when sysret will work, this is a prerequisite.  Otherwise
sysret will never match iret.  (I'm not immediately planning on doing
this, but I can imagine workloads (e.g. UML) for which it would be a
big improvement.)

--Andy

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

[H. Peter Anvin]

The real question is if we care that sysret and iter don't match.  On 32 bits the situation is even more complex.

On June 26, 2014 1:00:22 PM PDT, Andy Lutomirski <luto@amacapital.net> wrote:
>On Thu, Jun 26, 2014 at 12:59 PM, Andy Lutomirski <luto@amacapital.net>
>wrote:
>> On Thu, Jun 26, 2014 at 12:56 PM, Andi Kleen <andi@firstfloor.org>
>wrote:
>>>> show RCX == RIP even under strace.
>>>
>>> If you think it's really worth the extra instruction?
>>
>> Hard to say.  That extra instruction only happens on slow paths, so I
>> suspect the slowdown is negligible.  On the other hand, having
>syscall
>> show a blatant difference in behavior between traced and untraced
>> processes seems unfortunate.
>>
>>>
>>> It's not wrong, but it's not clear if it's useful.
>
>Also, if anyone ever wants to add some code to switch back from iret
>to sysret when sysret will work, this is a prerequisite.  Otherwise
>sysret will never match iret.  (I'm not immediately planning on doing
>this, but I can imagine workloads (e.g. UML) for which it would be a
>big improvement.)
>
>--Andy

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

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

[Andy Lutomirski]

On Thu, Jun 26, 2014 at 1:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> The real question is if we care that sysret and iter don't match.  On 32 bits the situation is even more complex.

At least for 64 bits, iret vs sysret is purely a kernel implementation
detail (except where a tracer modifies things that are inaccessible to
sysret), so ISTM it's worth one instruction to make them match.

I noticed this thing while fiddling with moving some of the syscall
tracing logic to C.  This isn't a real problem, but it at least made
me scratch my head.

--Andy

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

[Pavel Machek]

On Thu 2014-06-26 13:47:32, Andy Lutomirski wrote:
> On Thu, Jun 26, 2014 at 1:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > The real question is if we care that sysret and iter don't match.  On 32 bits the situation is even more complex.
> 
> At least for 64 bits, iret vs sysret is purely a kernel implementation
> detail (except where a tracer modifies things that are inaccessible to
> sysret), so ISTM it's worth one instruction to make them match.
> 
> I noticed this thing while fiddling with moving some of the syscall
> tracing logic to C.  This isn't a real problem, but it at least made
> me scratch my head.

If possible, we'd like to trace programs without programs being noticed they are 
being traced. See subterfugue utility, for example.

It is certainly worth one extra instruction.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Andy Lutomirski]

On Sat, Jun 28, 2014 at 10:07 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Thu 2014-06-26 13:47:32, Andy Lutomirski wrote:
>> On Thu, Jun 26, 2014 at 1:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> > The real question is if we care that sysret and iter don't match.  On 32 bits the situation is even more complex.
>>
>> At least for 64 bits, iret vs sysret is purely a kernel implementation
>> detail (except where a tracer modifies things that are inaccessible to
>> sysret), so ISTM it's worth one instruction to make them match.
>>
>> I noticed this thing while fiddling with moving some of the syscall
>> tracing logic to C.  This isn't a real problem, but it at least made
>> me scratch my head.
>
> If possible, we'd like to trace programs without programs being noticed they are
> being traced. See subterfugue utility, for example.
>
> It is certainly worth one extra instruction.

I tend to agree.

FWIW, I haven't looked at the ia32 stuff, but it should be possible to
do something similar if it's not there already.  The iret path can set
any user state it wants.

--Andy

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

[Andy Lutomirski]

On Thu, Jun 26, 2014 at 12:08 PM, Andy Lutomirski <luto@amacapital.net> 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@amacapital.net>
> ---
>  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