From: Helge Deller <deller@gmx.de>
To: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Oleg Nesterov <oleg@redhat.com>,
"James E.J. Bottomley" <jejb@parisc-linux.org>,
linux-parisc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] parisc: Fix ptrace syscall number modification
Date: Sat, 16 Feb 2019 17:55:24 +0100 [thread overview]
Message-ID: <c41a211b-6f61-b2a5-0613-32df52bfd367@gmx.de> (raw)
In-Reply-To: <20190216131039.GA18767@altlinux.org>
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. */
>
next prev parent reply other threads:[~2019-02-16 16:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-02-16 22:42 ` Dmitry V. Levin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c41a211b-6f61-b2a5-0613-32df52bfd367@gmx.de \
--to=deller@gmx.de \
--cc=jejb@parisc-linux.org \
--cc=ldv@altlinux.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=oleg@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).