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@xxxxxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxx>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxx>
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
