Re: [PATCH v8 3/3] selftests/ptrace: add a test case for PTRACE_SET_SYSCALL_INFO syscall skipping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/ptrace: tracee ASSERT_EQ() traps on write() before _exit()
- [Medium] selftests/ptrace: set_syscall_info_seccomp() fails instead of skipping
--

commit cc1ccfdbe8397159a506a6d7612652b6af1ea5d4
Author: Renzo Davoli <renzo@cs.unibo.it>

selftests/ptrace: add a test case for PTRACE_SET_SYSCALL_INFO syscall skipping

This commit adds a seccomp test case in set_syscall_info.c to verify
PTRACE_SET_SYSCALL_INFO syscall skipping semantics, including setting return
values, error values, and preventing syscall changes after skipping.

> diff --git a/tools/testing/selftests/ptrace/set_syscall_info.c b/tools/testing/selftests/ptrace/set_syscall_info.c
> index 1cc411a41cd6f..e8733c62177a9 100644
> --- a/tools/testing/selftests/ptrace/set_syscall_info.c
> +++ b/tools/testing/selftests/ptrace/set_syscall_info.c
[ ... ]
> @@ -516,4 +542,196 @@ TEST(set_syscall_info)
>  	ASSERT_EQ(ptrace_stop, ARRAY_SIZE(si) * 2);
>  }
>  
> +TEST(set_syscall_info_seccomp)
> +{
> +	tracer_pid = getpid();
> +	tracee_pid = fork();
> +
> +	ASSERT_LE(0, tracee_pid) {
> +		TH_LOG("fork: %m");
> +	}
> +
> +	/* tracee */
> +	if (tracee_pid == 0) {
> +		tracee_pid = getpid();
> +		ASSERT_EQ(0, sys_ptrace(PTRACE_TRACEME, 0, 0, 0)) {
> +			TH_LOG("PTRACE_TRACEME: %m");
> +		}
> +		ASSERT_EQ(0, kill(tracee_pid, SIGSTOP)) {
> +			/* cannot happen */
> +			TH_LOG("kill SIGSTOP: %m");
> +		}
> +
> +		ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> +			TH_LOG("prctl: %m");
> +		}
> +		ASSERT_EQ(0, sys_seccomp(SECCOMP_SET_MODE_FILTER, 0,
> +					(void *) &seccomp_prog)) {
> +			TH_LOG("seccomp: %m");
> +		}

[Severity: Medium]
Should set_syscall_info_seccomp skip the test instead of failing when
sys_seccomp(SECCOMP_SET_MODE_FILTER, ...) is not supported by the kernel or
architecture?

On kernels built without CONFIG_SECCOMP_FILTER (or on architectures that
support PTRACE_SET_SYSCALL_INFO via CONFIG_HAVE_ARCH_TRACEHOOK but lack
HAVE_ARCH_SECCOMP_FILTER), sys_seccomp() returns -1 with ENOSYS or EINVAL.

Because this ASSERT_EQ() runs inside the already-ptraced child, a failure
invokes __bail() -> abort(), which raises SIGABRT in the tracee and causes
the tracer to fail with "unexpected stop, wait status 0x67f" instead of
skipping the test via SKIP().

> +
> +		/* run getpid unmodified */
> +		ASSERT_EQ(tracee_pid, getpid()) {
> +			_exit(1);
> +		}
> +
> +		/* run getppid instead of getpid */
> +		ASSERT_EQ(tracer_pid, getpid()) {
> +			_exit(1);
> +		}
> +
> +		/* skip getpid and return 42 */
> +		ASSERT_EQ(42, getpid()) {
> +			_exit(1);
> +		}
> +
> +		/* skip getpid and fail with ESRCH */
> +		errno = 0;
> +		ASSERT_EQ(-1, syscall(__NR_getpid)) {
> +			_exit(1);
> +		}
> +		ASSERT_EQ(ESRCH, errno) {
> +			_exit(1);
> +		}
> +		_exit(0);
> +	}
[ ... ]
> +		ASSERT_LT(ptrace_stop, ARRAY_SIZE(expected_nr)) {
> +			LOG_KILL_TRACEE("ptrace stop overflow");
> +		}
> +		ASSERT_FALSE(info.seccomp.nr == __NR_exit_group && info.seccomp.args[0] == 1) {
> +			LOG_KILL_TRACEE("tracee error: %s",
> +					testname[ptrace_stop]);
> +		}
> +		ASSERT_EQ(info.seccomp.nr, expected_nr[ptrace_stop]) {
> +			LOG_KILL_TRACEE("syscall nr mismatch");
> +		}

[Severity: Medium]
If any of the getpid() checks in the tracee fail, will the tracee ever reach
_exit(1) to trigger the __NR_exit_group diagnostic check here?

In tools/testing/selftests/kselftest_harness.h, ASSERT_EQ() expands to
__EXPECT(), which logs to unbuffered stderr before running the optional
handler block:

#define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \
...
			__TH_LOG("Expected %s (%ju) %s %s (%ju)", \
				 _expected_str, __exp_print, #_t, \
				 _seen_str, __seen_print); \
...
} while (0); OPTIONAL_HANDLER(_assert)

Since seccomp_filter intercepts all syscalls other than __NR_restart_syscall
with SECCOMP_RET_TRACE, the fprintf(stderr, ...) inside __TH_LOG() triggers a
PTRACE_EVENT_SECCOMP stop on __NR_write before the { _exit(1); } block is
reached.

As a result, the tracer wakes up seeing info.seccomp.nr == __NR_write rather
than __NR_exit_group, bypasses the testname[ptrace_stop] diagnostic, fails
at ASSERT_EQ(info.seccomp.nr, expected_nr[ptrace_stop]) with "syscall nr
mismatch", and kills the tracee via LOG_KILL_TRACEE(). Plain if (...)
_exit(1); checks in the tracee (similar to TEST(set_syscall_info)) would
avoid issuing extra syscalls before _exit(1).

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020065635.490528-1-renzo@cs.unibo.it?part=3