| Message ID | 20190216131039.GA18767@altlinux.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] parisc: Fix ptrace syscall number modification | expand |
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. */ >
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!
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. */
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(-)