Re: [PATCH] selftests/seccomp: Enhance per-arch ptrace syscall skip tests

[Colin Ian King <colin.king@canonical.com>]

On 25/01/2019 18:33, Kees Cook wrote:
> Passing EPERM during syscall skipping was confusing since the test wasn't
> actually exercising the errno evaluation -- it was just passing a literal
> "1" (EPERM). Instead, expand the tests to check both direct value returns
> (positive, 45000 in this case), and errno values (negative, -ESRCH in this
> case) to check both fake success and fake failure during syscall skipping.
> 
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Fixes: a33b2d0359a0 ("selftests/seccomp: Add tests for basic ptrace actions")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Colin, does this end up working on s390? Based on your bug report, I
> suspect the positive value tests will fail, but the errno tests will
> pass. If that's true, I think something is wrong in the s390 handling.
> (And it may just be that the ptrace code to rewrite syscalls on s390
> in the test is wrong...) But splitting these tests up should tell us more.
> ---
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 72 +++++++++++++++----
>  1 file changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 496a9a8c773a..7e632b465ab4 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -1608,7 +1608,16 @@ TEST_F(TRACE_poke, getpid_runs_normally)
>  #ifdef SYSCALL_NUM_RET_SHARE_REG
>  # define EXPECT_SYSCALL_RETURN(val, action)	EXPECT_EQ(-1, action)
>  #else
> -# define EXPECT_SYSCALL_RETURN(val, action)	EXPECT_EQ(val, action)
> +# define EXPECT_SYSCALL_RETURN(val, action)		\
> +	do {						\
> +		errno = 0;				\
> +		if (val < 0) {				\
> +			EXPECT_EQ(-1, action);		\
> +			EXPECT_EQ(-(val), errno);	\
> +		} else {				\
> +			EXPECT_EQ(val, action);		\
> +		}					\
> +	} while (0)
>  #endif
>  
>  /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for
> @@ -1647,7 +1656,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
>  
>  /* Architecture-specific syscall changing routine. */
>  void change_syscall(struct __test_metadata *_metadata,
> -		    pid_t tracee, int syscall)
> +		    pid_t tracee, int syscall, int result)
>  {
>  	int ret;
>  	ARCH_REGS regs;
> @@ -1706,7 +1715,7 @@ void change_syscall(struct __test_metadata *_metadata,
>  #ifdef SYSCALL_NUM_RET_SHARE_REG
>  		TH_LOG("Can't modify syscall return on this architecture");
>  #else
> -		regs.SYSCALL_RET = EPERM;
> +		regs.SYSCALL_RET = result;
>  #endif
>  
>  #ifdef HAVE_GETREGS
> @@ -1734,14 +1743,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee,
>  	case 0x1002:
>  		/* change getpid to getppid. */
>  		EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee));
> -		change_syscall(_metadata, tracee, __NR_getppid);
> +		change_syscall(_metadata, tracee, __NR_getppid, 0);
>  		break;
>  	case 0x1003:
> -		/* skip gettid. */
> +		/* skip gettid with valid return code. */
>  		EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee));
> -		change_syscall(_metadata, tracee, -1);
> +		change_syscall(_metadata, tracee, -1, 45000);
>  		break;
>  	case 0x1004:
> +		/* skip openat with error. */
> +		EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee));
> +		change_syscall(_metadata, tracee, -1, -ESRCH);
> +		break;
> +	case 0x1005:
>  		/* do nothing (allow getppid) */
>  		EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee));
>  		break;
> @@ -1774,9 +1788,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
>  	nr = get_syscall(_metadata, tracee);
>  
>  	if (nr == __NR_getpid)
> -		change_syscall(_metadata, tracee, __NR_getppid);
> +		change_syscall(_metadata, tracee, __NR_getppid, 0);
> +	if (nr == __NR_gettid)
> +		change_syscall(_metadata, tracee, -1, 45000);
>  	if (nr == __NR_openat)
> -		change_syscall(_metadata, tracee, -1);
> +		change_syscall(_metadata, tracee, -1, -ESRCH);
>  }
>  
>  FIXTURE_DATA(TRACE_syscall) {
> @@ -1793,8 +1809,10 @@ FIXTURE_SETUP(TRACE_syscall)
>  		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002),
>  		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1),
>  		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003),
> -		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
> +		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1),
>  		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004),
> +		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005),
>  		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
>  	};
>  
> @@ -1842,15 +1860,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected)
>  	EXPECT_NE(self->mypid, syscall(__NR_getpid));
>  }
>  
> -TEST_F(TRACE_syscall, ptrace_syscall_dropped)
> +TEST_F(TRACE_syscall, ptrace_syscall_errno)
> +{
> +	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
> +	teardown_trace_fixture(_metadata, self->tracer);
> +	self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
> +					   true);
> +
> +	/* Tracer should skip the open syscall, resulting in ESRCH. */
> +	EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
> +}
> +
> +TEST_F(TRACE_syscall, ptrace_syscall_faked)
>  {
>  	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
>  	teardown_trace_fixture(_metadata, self->tracer);
>  	self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
>  					   true);
>  
> -	/* Tracer should skip the open syscall, resulting in EPERM. */
> -	EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat));
> +	/* Tracer should skip the gettid syscall, resulting fake pid. */
> +	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
>  }
>  
>  TEST_F(TRACE_syscall, syscall_allowed)
> @@ -1883,7 +1912,21 @@ TEST_F(TRACE_syscall, syscall_redirected)
>  	EXPECT_NE(self->mypid, syscall(__NR_getpid));
>  }
>  
> -TEST_F(TRACE_syscall, syscall_dropped)
> +TEST_F(TRACE_syscall, syscall_errno)
> +{
> +	long ret;
> +
> +	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> +	ASSERT_EQ(0, ret);
> +
> +	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0);
> +	ASSERT_EQ(0, ret);
> +
> +	/* openat has been skipped and an errno return. */
> +	EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
> +}
> +
> +TEST_F(TRACE_syscall, syscall_faked)
>  {
>  	long ret;
>  
> @@ -1894,8 +1937,7 @@ TEST_F(TRACE_syscall, syscall_dropped)
>  	ASSERT_EQ(0, ret);
>  
>  	/* gettid has been skipped and an altered return value stored. */
> -	EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid));
> -	EXPECT_NE(self->mytid, syscall(__NR_gettid));
> +	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
>  }
>  
>  TEST_F(TRACE_syscall, skip_after_RET_TRACE)
> 
Works perfectly. Thanks Kees.

Tested-by: Colin Ian King <colin.king@canonical.com>