linux-kernel - Re: [PATCH] x86: ptrace: sign-extend eax with orig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 22 Sep 2009 18:31:16 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Roland McGrath <roland@...hat.com>
cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, jan.kratochvil@...hat.com,
	Oleg Nesterov <oleg@...hat.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/