linux-kernel - Re: syscall_get_error() && TS

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:   Thu, 30 Mar 2017 15:51:00 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Jan Kratochvil <jan.kratochvil@...hat.com>,
        Pedro Alves <palves@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: syscall_get_error() && TS_ checks

On 03/29, Linus Torvalds wrote:
>
> On Wed, Mar 29, 2017 at 11:50 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > Again, afaics we only need these compat checks because regs->ax could be
> > changed by 32-bit debugger without sign-extension.
>
> You don't explain how you were planning on *fixing* that code. You
> know why it exists, but then you just say "let's remove it", without
> any explanation of what you'd replace it with.

Hmm. I tried to explain... Let me quote my initial email,

	So why we can't simply change putreg32() to always sign-extend regs->ax
	regs->orig_ax and just do

		static inline long syscall_get_error(struct task_struct *task,
						     struct pt_regs *regs)
		{
			return regs-ax;
		}

	? Or, better, simply kill it and use syscall_get_return_value() in
	arch/x86/kernel/signal.c.
                                                                                    	
	Of course, if the tracee is 64-bit and debugger is 32-bit then the
	unconditional sign-extend can be wrong, but do we really care about
	this case? This can't really work anyway. And the current code is not
	right too. Say, debugger nacks the signal which interrupted syscall
	and sets regs->ax = -ERESTARTSYS to force the restart, this won't work
	because TS_COMPAT|TS_I386_REGS_POKED are not set.

In short. can the patch below work?

Oleg.

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 9cc7d5a..96f21fc 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -917,11 +917,14 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
 	R32(edi, di);
 	R32(esi, si);
 	R32(ebp, bp);
-	R32(eax, ax);
 	R32(eip, ip);
 	R32(esp, sp);
 
 	case offsetof(struct user32, regs.orig_eax):
+		regs->ax = (long) (int) value;
+		break;
+
+	case offsetof(struct user32, regs.orig_eax):
 		/*
 		 * Warning: bizarre corner case fixup here.  A 32-bit
 		 * debugger setting orig_eax to -1 wants to disable
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index b3b98ff..41023f8 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -710,7 +710,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* If so, check system call restarting.. */
-		switch (syscall_get_error(current, regs)) {
+		switch (syscall_get_return_value(current, regs)) {
 		case -ERESTART_RESTARTBLOCK:
 		case -ERESTARTNOHAND:
 			regs->ax = -EINTR;
@@ -813,7 +813,7 @@ void do_signal(struct pt_regs *regs)
 	/* Did we come from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* Restart the system call - no handlers present */
-		switch (syscall_get_error(current, regs)) {
+		switch (syscall_get_return_value(current, regs)) {
 		case -ERESTARTNOHAND:
 		case -ERESTARTSYS:
 		case -ERESTARTNOINTR: