* [PATCH] parisc: Fix ptrace syscall number modification
@ 2019-02-16 2:46 Dmitry V. Levin
2019-02-16 13:10 ` [PATCH v2] " Dmitry V. Levin
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry V. Levin @ 2019-02-16 2:46 UTC (permalink / raw)
To: Helge Deller
Cc: Oleg Nesterov, James E.J. Bottomley, linux-parisc, linux-kernel
Commit 910cd32e552e ("parisc: Fix and enable seccomp filter support")
introduced a regression in ptrace-based syscall tampering: when tracer
changes syscall number to -1, the kernel fails to initialize %r28 with
-ENOSYS and subsequently fails to return the error code of the failed
syscall to userspace.
This erroneous behaviour could be observed with a simple strace syscall
fault injection command which is expected to print something like this:
$ strace -a0 -ewrite -einject=write:error=enospc echo hello
write(1, "hello\n", 6) = -1 ENOSPC (No space left on device) (INJECTED)
write(2, "echo: ", 6) = -1 ENOSPC (No space left on device) (INJECTED)
write(2, "write error", 11) = -1 ENOSPC (No space left on device) (INJECTED)
write(2, "\n", 1) = -1 ENOSPC (No space left on device) (INJECTED)
+++ exited with 1 +++
After commit 910cd32e552ea09caa89cdbe328e468979b030dd it loops printing
something like this instead:
write(1, "hello\n", 6../strace: Failed to tamper with process 12345: unexpectedly got no error (return value 0, error 0)
) = 0 (INJECTED)
This bug was found by strace test suite.
Fixes: 910cd32e552e ("parisc: Fix and enable seccomp filter support")
Cc: stable@vger.kernel.org # v4.5+
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
N.B. I have no parisc box to test the patch.
arch/parisc/kernel/ptrace.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index 2582df1c529b..9177f3e68b93 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -308,15 +308,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
long do_syscall_trace_enter(struct pt_regs *regs)
{
- if (test_thread_flag(TIF_SYSCALL_TRACE) &&
- 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;
+ if (test_thread_flag(TIF_SYSCALL_TRACE)) {
+ regs->gr[28] = -ENOSYS;
+ if (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;
+ return -1;
+ }
}
/* Do the secure computing check after ptrace. */
--
ldv
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2] parisc: Fix ptrace syscall number modification
2019-02-16 2:46 [PATCH] parisc: Fix ptrace syscall number modification Dmitry V. Levin
@ 2019-02-16 13:10 ` Dmitry V. Levin
2019-02-16 16:55 ` Helge Deller
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry V. Levin @ 2019-02-16 13:10 UTC (permalink / raw)
To: Helge Deller
Cc: Oleg Nesterov, James E.J. Bottomley, linux-parisc, linux-kernel
Commit 910cd32e552e ("parisc: Fix and enable seccomp filter support")
introduced a regression in ptrace-based syscall tampering: when tracer
changes syscall number to -1, the kernel fails to initialize %r28 with
-ENOSYS and subsequently fails to return the error code of the failed
syscall to userspace.
This erroneous behaviour could be observed with a simple strace syscall
fault injection command which is expected to print something like this:
$ strace -a0 -ewrite -einject=write:error=enospc echo hello
write(1, "hello\n", 6) = -1 ENOSPC (No space left on device) (INJECTED)
write(2, "echo: ", 6) = -1 ENOSPC (No space left on device) (INJECTED)
write(2, "write error", 11) = -1 ENOSPC (No space left on device) (INJECTED)
write(2, "\n", 1) = -1 ENOSPC (No space left on device) (INJECTED)
+++ exited with 1 +++
After commit 910cd32e552ea09caa89cdbe328e468979b030dd it loops printing
something like this instead:
write(1, "hello\n", 6../strace: Failed to tamper with process 12345: unexpectedly got no error (return value 0, error 0)
) = 0 (INJECTED)
This bug was found by strace test suite.
Fixes: 910cd32e552e ("parisc: Fix and enable seccomp filter support")
Cc: stable@vger.kernel.org # v4.5+
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
v2: Updated comments.
Set gr[28] to -ENOSYS after tracehook_report_syscall_entry() invocation
because setting of syscall return code by tracer on entering syscall
shall not affect the syscall return code.
N.B. I have no parisc box to test the patch.
arch/parisc/kernel/ptrace.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index 2582df1c529b..be4d6a279b12 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -308,15 +308,29 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
long do_syscall_trace_enter(struct pt_regs *regs)
{
- if (test_thread_flag(TIF_SYSCALL_TRACE) &&
- tracehook_report_syscall_entry(regs)) {
+ if (test_thread_flag(TIF_SYSCALL_TRACE)) {
+ int rc = 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.
+ * As tracesys_next does not set %r28 to -ENOSYS
+ * when %r20 is set to -1, initialize it here.
*/
- regs->gr[20] = -1UL;
- goto out;
+ regs->gr[28] = -ENOSYS;
+
+ if (rc) {
+ /*
+ * A nonzero return code from
+ * tracehook_report_syscall_entry() tells us
+ * to prevent the syscall execution. Skip
+ * the syscall call and the syscall restart handling.
+ *
+ * Note that the tracer may also just change
+ * regs->gr[20] to an invalid syscall number,
+ * that is handled by tracesys_next.
+ */
+ regs->gr[20] = -1UL;
+ return -1;
+ }
}
/* Do the secure computing check after ptrace. */
--
ldv
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] parisc: Fix ptrace syscall number modification
2019-02-16 13:10 ` [PATCH v2] " Dmitry V. Levin
@ 2019-02-16 16:55 ` Helge Deller
2019-02-16 22:42 ` Dmitry V. Levin
0 siblings, 1 reply; 4+ messages in thread
From: Helge Deller @ 2019-02-16 16:55 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, James E.J. Bottomley, linux-parisc, linux-kernel
On 16.02.19 14:10, Dmitry V. Levin wrote:
> Commit 910cd32e552e ("parisc: Fix and enable seccomp filter support")
> introduced a regression in ptrace-based syscall tampering: when tracer
> changes syscall number to -1, the kernel fails to initialize %r28 with
> -ENOSYS and subsequently fails to return the error code of the failed
> syscall to userspace.
>
> This erroneous behaviour could be observed with a simple strace syscall
> fault injection command which is expected to print something like this:
>
> $ strace -a0 -ewrite -einject=write:error=enospc echo hello
> write(1, "hello\n", 6) = -1 ENOSPC (No space left on device) (INJECTED)
> write(2, "echo: ", 6) = -1 ENOSPC (No space left on device) (INJECTED)
> write(2, "write error", 11) = -1 ENOSPC (No space left on device) (INJECTED)
> write(2, "\n", 1) = -1 ENOSPC (No space left on device) (INJECTED)
> +++ exited with 1 +++
>
> After commit 910cd32e552ea09caa89cdbe328e468979b030dd it loops printing
> something like this instead:
>
> write(1, "hello\n", 6../strace: Failed to tamper with process 12345: unexpectedly got no error (return value 0, error 0)
> ) = 0 (INJECTED)
>
> This bug was found by strace test suite.
>
> Fixes: 910cd32e552e ("parisc: Fix and enable seccomp filter support")
> Cc: stable@vger.kernel.org # v4.5+
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
Thanks, the patch works as expected.
You may add:
Tested-by: Helge Deller <deller@gmx.de>
There is an "out" label a few lines below, which should be removed as well.
Otherwise you get this warning:
arch/parisc/kernel/ptrace.c: In function ‘do_syscall_trace_enter’:
arch/parisc/kernel/ptrace.c:357:1: warning: label ‘out’ defined but not used [-Wunused-label]
I've fixed it up locally and added the patch to my for-next tree.
If it's ok for you, I'll push it through the parisc tree.
Helge
> ---
>
> v2: Updated comments.
> Set gr[28] to -ENOSYS after tracehook_report_syscall_entry() invocation
> because setting of syscall return code by tracer on entering syscall
> shall not affect the syscall return code.
>
> N.B. I have no parisc box to test the patch.
>
> arch/parisc/kernel/ptrace.c | 28 +++++++++++++++++++++-------
> 1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
> index 2582df1c529b..be4d6a279b12 100644
> --- a/arch/parisc/kernel/ptrace.c
> +++ b/arch/parisc/kernel/ptrace.c
> @@ -308,15 +308,29 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>
> long do_syscall_trace_enter(struct pt_regs *regs)
> {
> - if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> - tracehook_report_syscall_entry(regs)) {
> + if (test_thread_flag(TIF_SYSCALL_TRACE)) {
> + int rc = 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.
> + * As tracesys_next does not set %r28 to -ENOSYS
> + * when %r20 is set to -1, initialize it here.
> */
> - regs->gr[20] = -1UL;
> - goto out;
> + regs->gr[28] = -ENOSYS;
> +
> + if (rc) {
> + /*
> + * A nonzero return code from
> + * tracehook_report_syscall_entry() tells us
> + * to prevent the syscall execution. Skip
> + * the syscall call and the syscall restart handling.
> + *
> + * Note that the tracer may also just change
> + * regs->gr[20] to an invalid syscall number,
> + * that is handled by tracesys_next.
> + */
> + regs->gr[20] = -1UL;
> + return -1;
> + }
> }
>
> /* Do the secure computing check after ptrace. */
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] parisc: Fix ptrace syscall number modification
2019-02-16 16:55 ` Helge Deller
@ 2019-02-16 22:42 ` Dmitry V. Levin
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry V. Levin @ 2019-02-16 22:42 UTC (permalink / raw)
To: Helge Deller
Cc: Oleg Nesterov, James E.J. Bottomley, linux-parisc, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]
On Sat, Feb 16, 2019 at 05:55:24PM +0100, Helge Deller wrote:
> On 16.02.19 14:10, Dmitry V. Levin wrote:
> > Commit 910cd32e552e ("parisc: Fix and enable seccomp filter support")
> > introduced a regression in ptrace-based syscall tampering: when tracer
> > changes syscall number to -1, the kernel fails to initialize %r28 with
> > -ENOSYS and subsequently fails to return the error code of the failed
> > syscall to userspace.
> >
> > This erroneous behaviour could be observed with a simple strace syscall
> > fault injection command which is expected to print something like this:
> >
> > $ strace -a0 -ewrite -einject=write:error=enospc echo hello
> > write(1, "hello\n", 6) = -1 ENOSPC (No space left on device) (INJECTED)
> > write(2, "echo: ", 6) = -1 ENOSPC (No space left on device) (INJECTED)
> > write(2, "write error", 11) = -1 ENOSPC (No space left on device) (INJECTED)
> > write(2, "\n", 1) = -1 ENOSPC (No space left on device) (INJECTED)
> > +++ exited with 1 +++
> >
> > After commit 910cd32e552ea09caa89cdbe328e468979b030dd it loops printing
> > something like this instead:
> >
> > write(1, "hello\n", 6../strace: Failed to tamper with process 12345: unexpectedly got no error (return value 0, error 0)
> > ) = 0 (INJECTED)
> >
> > This bug was found by strace test suite.
> >
> > Fixes: 910cd32e552e ("parisc: Fix and enable seccomp filter support")
> > Cc: stable@vger.kernel.org # v4.5+
> > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
>
> Thanks, the patch works as expected.
> You may add:
> Tested-by: Helge Deller <deller@gmx.de>
>
> There is an "out" label a few lines below, which should be removed as well.
> Otherwise you get this warning:
> arch/parisc/kernel/ptrace.c: In function ‘do_syscall_trace_enter’:
> arch/parisc/kernel/ptrace.c:357:1: warning: label ‘out’ defined but not used [-Wunused-label]
>
> I've fixed it up locally and added the patch to my for-next tree.
> If it's ok for you, I'll push it through the parisc tree.
It's fine with me, thanks!
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-16 22:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-16 2:46 [PATCH] parisc: Fix ptrace syscall number modification Dmitry V. Levin
2019-02-16 13:10 ` [PATCH v2] " Dmitry V. Levin
2019-02-16 16:55 ` Helge Deller
2019-02-16 22:42 ` Dmitry V. Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).