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@xxxxxxxxxxxxxxx # v4.5+ > Signed-off-by: Dmitry V. Levin <ldv@xxxxxxxxxxxx> Thanks, the patch works as expected. You may add: Tested-by: Helge Deller <deller@xxxxxx> 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. */ >