parisc: Fix ptrace: syscall number and return value modification
diff mbox

Message ID 20160119150849.GA22546@p100.box
State Accepted, archived
Headers show

Commit Message

Helge Deller Jan. 19, 2016, 3:08 p.m. UTC
Mike Frysinger reported that his ptrace testcase showed strange
behaviour on parisc: It was not possible to avoid a syscall and the
return value of a syscall couldn't be changed.

To modify a syscall number, we were missing to save the new syscall
number to gr20 which is then picked up later in assembly again.

The effect that the return value couldn't be changed is a side-effect of
another bug in the assembly code. When a process is ptraced, userspace
expects each syscall to report entrance and exit of a syscall.  If a
syscall number was given which doesn't exist, we jumped to the normal
syscall exit code instead of informing userspace that the (non-existant)
syscall exits. This unexpected behaviour confuses userspace and thus the
bug was misinterpreted as if we can't change the return value.

This patch fixes both problems and was tested on 64bit kernel with
32bit userspace.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: <stable@vger.kernel.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mike Frysinger Jan. 19, 2016, 7:47 p.m. UTC | #1
On 19 Jan 2016 16:08, Helge Deller wrote:
> Mike Frysinger reported that his ptrace testcase showed strange
> behaviour on parisc: It was not possible to avoid a syscall and the
> return value of a syscall couldn't be changed.
> 
> To modify a syscall number, we were missing to save the new syscall
> number to gr20 which is then picked up later in assembly again.
> 
> The effect that the return value couldn't be changed is a side-effect of
> another bug in the assembly code. When a process is ptraced, userspace
> expects each syscall to report entrance and exit of a syscall.  If a
> syscall number was given which doesn't exist, we jumped to the normal
> syscall exit code instead of informing userspace that the (non-existant)
> syscall exits. This unexpected behaviour confuses userspace and thus the
> bug was misinterpreted as if we can't change the return value.
> 
> This patch fixes both problems and was tested on 64bit kernel with
> 32bit userspace.

thanks, i'll give it a spin on my box too

on a related note, can you check if arg reloading works too ?
i.e. i should also be able to mung syscall args on the fly.
e.g. if the tracee does open("/foo", O_RDWR), the tracer should
be able to munge it to do open("/foo", O_RDONLY).
-mike
Helge Deller Jan. 19, 2016, 8:13 p.m. UTC | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 19.01.2016 20:47, Mike Frysinger wrote:
> On 19 Jan 2016 16:08, Helge Deller wrote:
>> Mike Frysinger reported that his ptrace testcase showed strange
>> behaviour on parisc: It was not possible to avoid a syscall and the
>> return value of a syscall couldn't be changed.
>>
>> To modify a syscall number, we were missing to save the new syscall
>> number to gr20 which is then picked up later in assembly again.
>>
>> The effect that the return value couldn't be changed is a side-effect of
>> another bug in the assembly code. When a process is ptraced, userspace
>> expects each syscall to report entrance and exit of a syscall.  If a
>> syscall number was given which doesn't exist, we jumped to the normal
>> syscall exit code instead of informing userspace that the (non-existant)
>> syscall exits. This unexpected behaviour confuses userspace and thus the
>> bug was misinterpreted as if we can't change the return value.
>>
>> This patch fixes both problems and was tested on 64bit kernel with
>> 32bit userspace.
> 
> thanks, i'll give it a spin on my box too
> 
> on a related note, can you check if arg reloading works too ?
> i.e. i should also be able to mung syscall args on the fly.
> e.g. if the tracee does open("/foo", O_RDWR), the tracer should
> be able to munge it to do open("/foo", O_RDONLY).

I didn't tested it, but from looking at the assembly the user args get
reloaded from the task struct after having called do_syscall_trace_enter().
So, in theory it should work.

Helge
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJWnpjkAAoJEKGDlV8wpRJB5G4P/3FgRUNuPKs/zfTKMuvM7o4e
WX6dyBGGUJAghRLaqX9nrCTsE78Bg2BNT3sPOZ13Zac6hjxoxhCAJz0J0iuVkTpY
dTiZAgqLzErsTQr5WpQCVrbCE5qF09/7HGItOWAFn4GRurqIehId0k7vRmHDmb24
L5/lJzoTTll+8UO6k5MyOfHXyOLRiK+Ci0BMCXsi8b5NCCPBuMQoErwowrVyMSz9
2YFIbdWdHjUwwFruodn9rOQ+tNzeVL2uQbPCj6tyy7iIx7rEiYutrc3bz3SDl7in
MoFRurF6hjeRl38EjgkwgD52V5y9P2mJkhiTj3hxnUvKbTiE7Dhm7dxeGyRlMJ3L
TJIHhOhbO8GaPkeDrxhwCJV89oTHKN7l2YxTHjadgiJA9rSyR9R93cC9ltqy/5we
+y/JmUy+zohD9A12hwPfdU7srkMfHML/Uphu/R4D/LALOmWDm1U7+gistvRl1qhk
wECzweYw34nRXDsXxdI/Ey2YmdPVOfLxHtWtTokdNGNCwHsObDbOJ/lG2dx68qwA
b0w/IasML7ziqdtgUxsYePjG+IKynlS1qTqbFiGTxYa9vXUP3u4qSEIk0MDoMdxb
98CQYCG625gbMbqh8TSMgdh8qTt/1LMGh8Oh8QugWec+g7IpViHoFpkkrX4StGt+
GHHaM53rPq0Os65vAL67
=S6Ir
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Frysinger Jan. 20, 2016, 5:09 p.m. UTC | #3
On 19 Jan 2016 16:08, Helge Deller wrote:
> Mike Frysinger reported that his ptrace testcase showed strange
> behaviour on parisc: It was not possible to avoid a syscall and the
> return value of a syscall couldn't be changed.
> 
> To modify a syscall number, we were missing to save the new syscall
> number to gr20 which is then picked up later in assembly again.
> 
> The effect that the return value couldn't be changed is a side-effect of
> another bug in the assembly code. When a process is ptraced, userspace
> expects each syscall to report entrance and exit of a syscall.  If a
> syscall number was given which doesn't exist, we jumped to the normal
> syscall exit code instead of informing userspace that the (non-existant)
> syscall exits. This unexpected behaviour confuses userspace and thus the
> bug was misinterpreted as if we can't change the return value.
> 
> This patch fixes both problems and was tested on 64bit kernel with
> 32bit userspace.

i've checked that i can reset the syscall # fine and mung the return
value.  i haven't checked arg replacement although i don't need that
myself (yet?).  thanks !

Tested-by: Mike Frysinger <vapier@gentoo.org>
-mike

Patch
diff mbox

diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index 9585c81..ce0b2b4 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -269,14 +269,19 @@  long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 
 long do_syscall_trace_enter(struct pt_regs *regs)
 {
-	long ret = 0;
-
 	/* Do the secure computing check first. */
 	secure_computing_strict(regs->gr[20]);
 
 	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
-	    tracehook_report_syscall_entry(regs))
-		ret = -1L;
+	    tracehook_report_syscall_entry(regs)) {
+		/*
+		 * Tracing decided this syscall should not happen or the
+		 * debugger stored an invalid system call number. Skip
+		 * the system call and the system call restart handling.
+		 */
+		regs->gr[20] = -1UL;
+		goto out;
+	}
 
 #ifdef CONFIG_64BIT
 	if (!is_compat_task())
@@ -290,7 +295,8 @@  long do_syscall_trace_enter(struct pt_regs *regs)
 			regs->gr[24] & 0xffffffff,
 			regs->gr[23] & 0xffffffff);
 
-	return ret ? : regs->gr[20];
+out:
+	return regs->gr[20];
 }
 
 void do_syscall_trace_exit(struct pt_regs *regs)
diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
index 3fbd725..fbafa0d 100644
--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -343,7 +343,7 @@  tracesys_next:
 #endif
 
 	comiclr,>>=	__NR_Linux_syscalls, %r20, %r0
-	b,n	.Lsyscall_nosys
+	b,n	.Ltracesys_nosys
 
 	LDREGX  %r20(%r19), %r19
 
@@ -359,6 +359,9 @@  tracesys_next:
 	be      0(%sr7,%r19)
 	ldo	R%tracesys_exit(%r2),%r2
 
+.Ltracesys_nosys:
+	ldo	-ENOSYS(%r0),%r28		/* set errno */
+
 	/* Do *not* call this function on the gateway page, because it
 	makes a direct call to syscall_trace. */