[PATCH] x86-64: ptrace ia32 BP fix

[PATCH] x86-64: ptrace ia32 BP fix

[Roland McGrath]

When the 32-bit vDSO is used to make a system call, the %ebp register for
the 6th syscall arg has to be loaded from the user stack (where it's pushed
by the vDSO user code).  The native i386 kernel always does this before
stopping for syscall tracing, so %ebp can be seen and modified via ptrace
to access the 6th syscall argument.  The x86-64 kernel fails to do this,
presenting the stack address to ptrace instead.  This makes the %rbp value
seen by 64-bit ptrace of a 32-bit process, and the %ebp value seen by a
32-bit caller of ptrace, both differ from the native i386 behavior.

This patch fixes the problem by putting the word loaded from the user stack
into %rbp before calling syscall_trace_enter, and reloading the 6th syscall
argument from there afterwards (so ptrace can change it).  This makes the
behavior match that of i386 kernels.

Signed-off-by: Roland McGrath <roland@redhat.com>

--- a/arch/x86_64/ia32/ia32entry.S
+++ b/arch/x86_64/ia32/ia32entry.S
@@ -102,6 +102,7 @@ sysenter_do_call:	
 	.byte	0xf, 0x35
 
 sysenter_tracesys:
+	movl	%r9d,%ebp
 	SAVE_REST
 	CLEAR_RREGS
 	movq	$-ENOSYS,RAX(%rsp)	/* really needed? */
@@ -109,13 +110,7 @@ sysenter_tracesys:
 	call	syscall_trace_enter
 	LOAD_ARGS ARGOFFSET  /* reload args from stack in case ptrace changed it */
 	RESTORE_REST
-	movl	%ebp, %ebp
-	/* no need to do an access_ok check here because rbp has been
-	   32bit zero extended */ 
-1:	movl	(%rbp),%r9d
-	.section __ex_table,"a"
-	.quad 1b,ia32_badarg
-	.previous
+	movl	%ebp,%r9d
 	jmp	sysenter_do_call
 	CFI_ENDPROC
 
@@ -183,6 +178,7 @@ cstar_do_call:	
 	sysretl
 	
 cstar_tracesys:	
+	movl %r9d,%ebp
 	SAVE_REST
 	CLEAR_RREGS
 	movq $-ENOSYS,RAX(%rsp)	/* really needed? */
@@ -191,12 +187,7 @@ cstar_tracesys:	
 	LOAD_ARGS ARGOFFSET  /* reload args from stack in case ptrace changed it */
 	RESTORE_REST
 	movl RSP-ARGOFFSET(%rsp), %r8d
-	/* no need to do an access_ok check here because r8 has been
-	   32bit zero extended */ 
-1:	movl	(%r8),%r9d
-	.section __ex_table,"a"
-	.quad 1b,ia32_badarg
-	.previous
+	movl %ebp,%r9d
 	jmp cstar_do_call
 				
 ia32_badarg:

Re: [PATCH] x86-64: ptrace ia32 BP fix

[Daniel Jacobowitz]

On Tue, Jul 05, 2005 at 02:31:15AM -0700, Roland McGrath wrote:
> 
> When the 32-bit vDSO is used to make a system call, the %ebp register for
> the 6th syscall arg has to be loaded from the user stack (where it's pushed
> by the vDSO user code).  The native i386 kernel always does this before
> stopping for syscall tracing, so %ebp can be seen and modified via ptrace
> to access the 6th syscall argument.  The x86-64 kernel fails to do this,
> presenting the stack address to ptrace instead.  This makes the %rbp value
> seen by 64-bit ptrace of a 32-bit process, and the %ebp value seen by a
> 32-bit caller of ptrace, both differ from the native i386 behavior.
> 
> This patch fixes the problem by putting the word loaded from the user stack
> into %rbp before calling syscall_trace_enter, and reloading the 6th syscall
> argument from there afterwards (so ptrace can change it).  This makes the
> behavior match that of i386 kernels.

Wouldn't this  to botch a debugger which supported both backtracing and
PTRACE_SYSCALL, when stopped in a syscall?  We have unwind information
for the VDSO and it's not going to tell us that the kernel has done
something clever to the value of %ebp.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] x86-64: ptrace ia32 BP fix

[Andi Kleen]

> Wouldn't this  to botch a debugger which supported both backtracing and
> PTRACE_SYSCALL, when stopped in a syscall?  We have unwind information
> for the VDSO and it's not going to tell us that the kernel has done
> something clever to the value of %ebp.

The kernel is indeed not supposed to modify any input
registers of syscalls (ok except rcx, but that is unavoidable)

-Andi

Re: [PATCH] x86-64: ptrace ia32 BP fix

[Roland McGrath]

> Wouldn't this  to botch a debugger which supported both backtracing and
> PTRACE_SYSCALL, when stopped in a syscall?  We have unwind information
> for the VDSO and it's not going to tell us that the kernel has done
> something clever to the value of %ebp.

The user vDSO code pushes %ebp on the stack and then clobbers it.  The
unwind information says that %ebp was clobbered and says where the original
value can be found on the stack.  Unwinding doesn't care whether the %ebp
value is the one clobbered by the vDSO code, or the one clobbered by the
kernel (back to the previous value).  Like I said before, this matches the
i386 behavior.  If this were a problem, then it would have broken unwinding
on i386 already, but it's not a problem.

> The kernel is indeed not supposed to modify any input
> registers of syscalls (ok except rcx, but that is unavoidable)

The kernel plus the vDSO code together yield no modification.  That's the
point.  The kernel modifies %ebp in the sysenter/syscall path to match what
it would have contained if the user had done int $0x80 instead of call
vDSO.  This is the most sensible thing.  Otherwise using the unwind
information to back out of the vDSO special-frame would be required by every
tracer that wants to know the 6th argument to a system call.  (Or else it
would have to recognize the vDSO entry magically and have hard-wired
knowledge of what that does with the stack and registers.)

This change really does what I said: it makes the behavior consistent with
the i386 behavior.  Even if that were wrong (which it's not really), the
principle remains that the x86-64 support for 32-bit processes should
behave like the real 32-bit kernel does.


Thanks,
Roland

Re: [PATCH] x86-64: ptrace ia32 BP fix

[Roland McGrath]

> On Tue, Jul 05, 2005 at 02:31:15AM -0700, Roland McGrath wrote:
> > --- a/arch/x86_64/ia32/ia32entry.S
> > +++ b/arch/x86_64/ia32/ia32entry.S
> > @@ -102,6 +102,7 @@ sysenter_do_call:	
> >  	.byte	0xf, 0x35
> >  
> >  sysenter_tracesys:
> > +	movl	%r9d,%ebp
> >  	SAVE_REST
> 
> This is wrong because it will clobber ebp before it is saved.
> It is only saved in SAVE_REST.

It is right because it stores %ebp before it is saved in the argument block
that ptrace can access.  That is the point of it.  %r9d has the value
loaded from (%rbp) just prior to this code, which is what %ebp should
reflect to match the i386 behavior.

> 
> >  	CLEAR_RREGS
> >  	movq	$-ENOSYS,RAX(%rsp)	/* really needed? */
> > @@ -109,13 +110,7 @@ sysenter_tracesys:
> >  	call	syscall_trace_enter
> >  	LOAD_ARGS ARGOFFSET  /* reload args from stack in case ptrace changed it */
> >  	RESTORE_REST
> > -	movl	%ebp, %ebp
> > -	/* no need to do an access_ok check here because rbp has been
> > -	   32bit zero extended */ 
> > -1:	movl	(%rbp),%r9d
> > -	.section __ex_table,"a"
> > -	.quad 1b,ia32_badarg
> > -	.previous
> > +	movl	%ebp,%r9d
> 
> And this also cannot be correct because RESTORE_REST has overwritten %rbp
> already.

This is also correct because RESTORE_REST has stored into %rbp the value in
the argument block, which ptrace may have modified.  This movl ensures that
this changed value is what the system call's argument will be.

The patch is tested and works.  Just try strace on a 32-bit program that
calls mmap2 and look at the 6th argument value.  It shows a stack address
without this patch.  With this patch, it shows the argument value the same
as strace on a native i386 kernel does.


Thanks,
Roland