Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 3 Jul 2020 09:39:14 +0100
From:   Will Deacon <will@kernel.org>
To:     Keno Fischer <keno@juliacomputing.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Oleg Nesterov <oleg@redhat.com>,
        Kees Cook <keescook@chromium.org>,
        Andy Lutomirski <luto@amacapital.net>,
        Will Drewry <wad@chromium.org>
Subject: Re: ptrace: seccomp: Return value when the call was already invalid
Message-ID: <20200703083914.GA18516@willie-the-truck>
References: <CABV8kRxA9mXPZwtYrjbAfOfFewhABHddipccgk-LQJO+ZYu4Xg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CABV8kRxA9mXPZwtYrjbAfOfFewhABHddipccgk-LQJO+ZYu4Xg@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);