On Fri, Sep 11, 2020 at 03:10:12PM -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.
This looks like two fixes in one, so this should be split. :)
> tools/testing/selftests/seccomp/seccomp_bpf.c | 81 ++++++++++++-------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 7a6d40286a42..0ddc0846e9c0 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -1837,15 +1837,24 @@ void change_syscall(struct __test_metadata *_metadata,
> #endif
>
> /* If syscall is skipped, change return value. */
> - if (syscall == -1)
> + if (syscall == -1) {
> #ifdef SYSCALL_NUM_RET_SHARE_REG
> TH_LOG("Can't modify syscall return on this architecture");
> -
> #elif defined(__xtensa__)
> regs.SYSCALL_RET(regs) = result;
> +#elif defined(__powerpc__)
> + /* Error is signaled by CR0 SO bit and error code is positive. */
> + if (result < 0) {
> + regs.SYSCALL_RET = -result;
> + regs.ccr |= 0x10000000;
> + } else {
> + regs.SYSCALL_RET = result;
> + regs.ccr &= ~0x10000000;
> + }
> #else
> regs.SYSCALL_RET = result;
> #endif
> + }
I'll send a series soon that will include this bit, since I don't want
to collect these kinds of arch-specific things in the functions. (And
the xtensa one went in without my review!)
> +FIXTURE(TRACE_syscall) {
> + struct sock_fprog prog;
> + pid_t tracer, mytid, mypid, parent;
> +};
> +
> +FIXTURE_VARIANT(TRACE_syscall) {
> + /*
> + * All of the SECCOMP_RET_TRACE behaviors can be tested with either
> + * SECCOMP_RET_TRACE+PTRACE_CONT or plain ptrace()+PTRACE_SYSCALL.
> + * This indicates if we should use SECCOMP_RET_TRACE (false), or
> + * ptrace (true).
> + */
> + bool use_ptrace;
> +
> + /*
> + * Some archs (like ppc) only support changing the return code during
> + * syscall exit when ptrace is used. As the syscall number might not
> + * be available anymore during syscall exit, it needs to be saved
> + * during syscall enter.
> + */
> + int syscall_nr;
This should be part of the fixture struct, not the variant.
> +};
> +
> +FIXTURE_VARIANT_ADD(TRACE_syscall, ptrace) {
> + .use_ptrace = true,
> +};
> +
> +FIXTURE_VARIANT_ADD(TRACE_syscall, seccomp) {
> + .use_ptrace = false,
> +};
i.e. if a member isn't initialized in FIXTURE_VARIANT_ADD, it shouldn't
be defined in FIXTURE_VARIANT. :)
> +
> void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
> int status, void *args)
> {
> int ret, nr;
> unsigned long msg;
> static bool entry;
> + FIXTURE_VARIANT(TRACE_syscall) * variant = args;
>
> /*
> * The traditional way to tell PTRACE_SYSCALL entry/exit
> @@ -1916,10 +1957,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 && !variant)
> return;
>
> - nr = get_syscall(_metadata, tracee);
> + if (entry)
> + nr = get_syscall(_metadata, tracee);
> + else if (variant)
> + nr = variant->syscall_nr;
> + if (variant)
> + variant->syscall_nr = nr;
So, to be clear this is _only_ an issue for the ptrace side of things,
yes? i.e. seccomp's setting of the return value will correct stick?
--
Kees Cook
