diff mbox series

[13/15] selftests/seccomp: powerpc: Set syscall return during ptrace syscall exit

Message ID 20200912110820.597135-14-keescook@chromium.org
State New
Headers show
Series None | expand

Commit Message

Kees Cook Sept. 12, 2020, 11:08 a.m. UTC 
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. Adjust the ptrace tests to do this.

Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Suggested-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Link: https://lore.kernel.org/linux-kselftest/20200911181012.171027-1-cascardo@canonical.com/
Fixes: 58d0a862f573 ("seccomp: add tests for ptrace hole")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 34 +++++++++++--------
 1 file changed, 20 insertions(+), 14 deletions(-)

Comments

Kees Cook Sept. 14, 2020, 8:20 p.m. UTC | #1 
On Mon, Sep 14, 2020 at 03:47:13PM +1000, Michael Ellerman wrote:
> Kees Cook <keescook@chromium.org> writes:

> > 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. Adjust the ptrace tests to do this.

> 

> I'm not that across all the fixture stuff, but if I'm reading it right

> you're now calling change_syscall() on both entry and exit for all

> arches.


Correct.

> That should work, but it no longer tests changing the return code on

> entry on the arches that support it, which seems like a backward step?


That's a good point. I wouldn't be in a position to notice a regression
for the other architectures. I will refactor this one...

-- 
Kees Cook
diff mbox series

Patch

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index bbab2420d708..26c712c6a575 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -1949,12 +1949,19 @@  void tracer_seccomp(struct __test_metadata *_metadata, pid_t tracee,
 
 }
 
+FIXTURE(TRACE_syscall) {
+	struct sock_fprog prog;
+	pid_t tracer, mytid, mypid, parent;
+	long syscall_nr;
+};
+
 void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
 		   int status, void *args)
 {
-	int ret, nr;
+	int ret;
 	unsigned long msg;
 	static bool entry;
+	FIXTURE_DATA(TRACE_syscall) *self = args;
 
 	/*
 	 * The traditional way to tell PTRACE_SYSCALL entry/exit
@@ -1968,24 +1975,23 @@  void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
 	EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
 			: PTRACE_EVENTMSG_SYSCALL_EXIT, msg);
 
-	if (!entry)
-		return;
-
-	nr = get_syscall(_metadata, tracee);
+	/*
+	 * Some architectures only support setting return values during
+	 * syscall exit under ptrace, and on exit the syscall number may
+	 * no longer be available. Therefore, save it here, and call
+	 * "change syscall and set return values" on both entry and exit.
+	 */
+	if (entry)
+		self->syscall_nr = get_syscall(_metadata, tracee);
 
-	if (nr == __NR_getpid)
+	if (self->syscall_nr == __NR_getpid)
 		change_syscall(_metadata, tracee, __NR_getppid, 0);
-	if (nr == __NR_gettid)
+	if (self->syscall_nr == __NR_gettid)
 		change_syscall(_metadata, tracee, -1, 45000);
-	if (nr == __NR_openat)
+	if (self->syscall_nr == __NR_openat)
 		change_syscall(_metadata, tracee, -1, -ESRCH);
 }
 
-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
@@ -2044,7 +2050,7 @@  FIXTURE_SETUP(TRACE_syscall)
 	self->tracer = setup_trace_fixture(_metadata,
 					   variant->use_ptrace ? tracer_ptrace
 							       : tracer_seccomp,
-					   NULL, variant->use_ptrace);
+					   self, variant->use_ptrace);
 
 	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
 	ASSERT_EQ(0, ret);