From: Kees Cook <keescook@chromium.org>
To: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Cc: linux-kselftest@vger.kernel.org, Oleg Nesterov <oleg@redhat.com>,
Shuah Khan <shuah@kernel.org>,
linuxppc-dev@lists.ozlabs.org,
Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH] selftests/seccomp: fix ptrace tests on powerpc
Date: Tue, 8 Sep 2020 16:18:17 -0700 [thread overview]
Message-ID: <202009081505.D9FE52510B@keescook> (raw)
In-Reply-To: <20200630164739.1268222-1-cascardo@canonical.com>
On Tue, Jun 30, 2020 at 01:47:39PM -0300, Thadeu Lima de Souza Cascardo wrote:
> As pointed out by Michael Ellerman, the ptrace ABI on powerpc does not
> allow or require the return code to be set on syscall entry when
> skipping the syscall. It will always return ENOSYS and the return code
> must be set on syscall exit.
>
> This code does that, behaving more similarly to strace. It still sets
> the return code on entry, which is overridden on powerpc, and it will
> always repeat the same on exit. Also, on powerpc, the errno is not
> inverted, and depends on ccr.so being set.
>
> This has been tested on powerpc and amd64.
>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Kees Cook <keescook@google.com>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Yikes, I missed this from a while ago. I apologize for responding so
late!
This appears still unfixed; is that correct?
> ---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 24 +++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 252140a52553..b90a9190ba88 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -1738,6 +1738,14 @@ void change_syscall(struct __test_metadata *_metadata,
> TH_LOG("Can't modify syscall return on this architecture");
> #else
> regs.SYSCALL_RET = result;
> +# if defined(__powerpc__)
> + if (result < 0) {
> + regs.SYSCALL_RET = -result;
> + regs.ccr |= 0x10000000;
> + } else {
> + regs.ccr &= ~0x10000000;
> + }
> +# endif
> #endif
Just so I understand correctly: for ppc to "see" this result, it needs
to be both negative AND have this specific register set?
>
> #ifdef HAVE_GETREGS
> @@ -1796,6 +1804,7 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
> int ret, nr;
> unsigned long msg;
> static bool entry;
> + int *syscall_nr = args;
>
> /*
> * The traditional way to tell PTRACE_SYSCALL entry/exit
> @@ -1809,10 +1818,15 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
> EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
> : PTRACE_EVENTMSG_SYSCALL_EXIT, msg);
>
> - if (!entry)
> + if (!entry && !syscall_nr)
> return;
>
> - nr = get_syscall(_metadata, tracee);
> + if (entry)
> + nr = get_syscall(_metadata, tracee);
> + else
> + nr = *syscall_nr;
This is weird? Shouldn't get_syscall() be modified to do the right thing
here instead of depending on the extra arg?
> + if (syscall_nr)
> + *syscall_nr = nr;
>
> if (nr == __NR_getpid)
> change_syscall(_metadata, tracee, __NR_getppid, 0);
> @@ -1889,9 +1903,10 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected)
>
> TEST_F(TRACE_syscall, ptrace_syscall_errno)
> {
> + int syscall_nr = -1;
> /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
> teardown_trace_fixture(_metadata, self->tracer);
> - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
> + self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, &syscall_nr,
> true);
>
> /* Tracer should skip the open syscall, resulting in ESRCH. */
> @@ -1900,9 +1915,10 @@ TEST_F(TRACE_syscall, ptrace_syscall_errno)
>
> TEST_F(TRACE_syscall, ptrace_syscall_faked)
> {
> + int syscall_nr = -1;
> /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
> teardown_trace_fixture(_metadata, self->tracer);
> - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
> + self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, &syscall_nr,
> true);
>
> /* Tracer should skip the gettid syscall, resulting fake pid. */
> --
> 2.25.1
>
--
Kees Cook
next prev parent reply other threads:[~2020-09-08 23:18 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-30 16:47 [PATCH] selftests/seccomp: fix ptrace tests on powerpc Thadeu Lima de Souza Cascardo
2020-09-08 23:18 ` Kees Cook [this message]
2020-09-11 18:06 ` Thadeu Lima de Souza Cascardo
2020-09-13 12:34 ` Michael Ellerman
2020-09-17 22:37 ` Kees Cook
2020-09-17 22:51 ` Thadeu Lima de Souza Cascardo
2020-09-18 6:22 ` Michael Ellerman
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=202009081505.D9FE52510B@keescook \
--to=keescook@chromium.org \
--cc=cascardo@canonical.com \
--cc=linux-kselftest@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=oleg@redhat.com \
--cc=shuah@kernel.org \
/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).