lkml.org 
[lkml]   [2009]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] x86: ptrace: sign-extend eax with orig_eax>=0


On Tue, 22 Sep 2009, Roland McGrath wrote:
>
> The 32-bit ptrace syscall on a 64-bit kernel (32-bit debugger on
> 32-bit task) behaves differently than a native 32-bit kernel. When
> setting a register state of orig_eax>=0 and eax=-ERESTART* when the
> debugged task is NOT on its way out of a 32-bit syscall, the task will
> fail to do the syscall restart logic that it should do.

Hmm. This really smells extremely hacky to me.

I see what you're doing, and I understand why, but it all makes me
shudder. I think we already do the wrong thing for 'orig_ax', and that we
should probably simply test just the low 32 bits of it (looks to be easily
done by just making the return type of 'syscall_get_nr()' be 'int') rather
than have that special "let's sign-extend orig_ax" code in ptrace.

I also get the feeling that the TS_COMPAT testing is just hacky to begin
with. I think it was broken to do things that way, and I have this gut
feel that we really should have hidden the "am I a 32-bit or 64-bit
syscall" in that orig_ax field instead (ie make the 64-bit system calls
set a bit or whatever). That's the field we've always used for system call
flagging, and TS_COMPAT was broken.

In this example, the problem for ptrace seems to be that TS_COMPAT ends up
being that "extra" bit of information that isn't visible in the register
set.

But that's a bigger separate cleanup. In the meantime, I really get the
feeling that your patch is nasty, and could be replaced with something
like the following instead.. It just sets the TS_COMPAT bit if we use a
32-bit interface to set the system call number to positive (ie "inside
system call").

THE BELOW IS TOTALLY UNTESTED! I haven't really thought it through. I just
reacted very negatively to your patch, and my gut feel is that the code is
fundamentally doing something wrong. The below may not work, and may be
seriously broken and miss the point, but I think it conceptually comes
closer to how things _should_ work.

Linus

---
arch/x86/include/asm/syscall.h | 2 +-
arch/x86/kernel/ptrace.c | 8 +++++++-
2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index d82f39b..f2a0631 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -16,7 +16,7 @@
#include <linux/sched.h>
#include <linux/err.h>

-static inline long syscall_get_nr(struct task_struct *task,
+static inline int syscall_get_nr(struct task_struct *task,
struct pt_regs *regs)
{
/*
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 8d7d5c9..90b67db 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1124,13 +1124,19 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
R32(eip, ip);
R32(esp, sp);

- case offsetof(struct user32, regs.orig_eax):
+ case offsetof(struct user32, regs.orig_eax): {
+ struct thread_info *thread = task_thread_info(child);
/*
* Sign-extend the value so that orig_eax = -1
* causes (long)orig_ax < 0 tests to fire correctly.
*/
regs->orig_ax = (long) (s32) value;
+ if ((s32) value >= 0)
+ thread->status |= TS_COMPAT;
+ else
+ thread->status &= ~TS_COMPAT;
break;
+ }

case offsetof(struct user32, regs.eflags):
return set_flags(child, value);

\
 
 \ /
  Last update: 2009-10-18 23:28    [W:0.062 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site