On Fri, Jul 03, 2020 at 09:39:14AM +0100, Will Deacon wrote:
> Hi Keno,
>
> On Fri, May 22, 2020 at 09:01:01PM -0400, Keno Fischer wrote:
> > I'm seeing the following while porting a ptracer from
> > x86_64 to arm64 (cc'ing arm64 folks, but in this case
> > x86_64 is the odd one out, I think other archs would
> > be consistent with arm64).
> >
> > Consider userspace code like the following:
> > ```
> > int ret = syscall(-10, 0);
> > assert(ret == -ENOSYS);
> > ```
> >
> > (Never mind the fact that this is something userspace
> > shouldn't do, I saw this in our test suite that tests
> > corner cases where the ptracer shouldn't affect behavior).
> >
> > Now, if we have a seccomp filter that simply does
> > SECCOMP_RET_TRACE, and a ptracer that simply
> > does PTRACE_CONT
>
> Ok, so this means that we're _skipping_ the system call, right?
>
> > then the assert will fire/fail on arm64, but not on x86_64.
>
> It feels weird to me that skipping the system call has any effect on the
> tracee registers...
>
> > Interestingly, arm64 does do something different
> > if the syscall is -1 rather than -10, where early
> > in the ptrace stop it does.
> > ```
> > /* set default errno for user-issued syscall(-1) */
> > if (scno == NO_SYSCALL)
> > regs->regs[0] = -ENOSYS;
>
> ... so I think this should be fixed too. How about the diff below?
>
> Will
>
> --->8
>
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 68b7f34a08f5..cb3f653c9688 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1833,12 +1833,12 @@ int syscall_trace_enter(struct pt_regs *regs)
> if (flags & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
> if (!in_syscall(regs) || (flags & _TIF_SYSCALL_EMU))
> - return -1;
> + return -ENOSYS;
> }
>
> /* Do the secure computing after ptrace; failures should be fast. */
> if (secure_computing() == -1)
> - return -1;
> + return -ENOSYS;
>
> if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
> trace_sys_enter(regs, regs->syscallno);
> @@ -1846,7 +1846,7 @@ int syscall_trace_enter(struct pt_regs *regs)
> audit_syscall_entry(regs->syscallno, regs->orig_x0, regs->regs[1],
> regs->regs[2], regs->regs[3]);
>
> - return regs->syscallno;
> + return 0;
> }
>
> void syscall_trace_exit(struct pt_regs *regs)
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index 5f5b868292f5..a13661f44818 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -121,12 +121,10 @@ static void el0_svc_common(struct pt_regs *regs, int
> scno, int sc_nr,
> user_exit();
>
> if (has_syscall_work(flags)) {
> - /* set default errno for user-issued syscall(-1) */
> - if (scno == NO_SYSCALL)
> - regs->regs[0] = -ENOSYS;
> - scno = syscall_trace_enter(regs);
> - if (scno == NO_SYSCALL)
> + if (syscall_trace_enter(regs))
> goto trace_exit;
> +
> + scno = regs->syscallno;
> }
>
> invoke_syscall(regs, scno, sc_nr, syscall_table);
What effect do either of these patches have on the existing seccomp
selftests: tools/testing/selftests/seccomp/seccomp_bpf ?
--
Kees Cook
