Re: ptrace: seccomp: Return value when the call was already invalid

[Kees Cook]

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