Re: [PATCH v2 3/3] selftest: seccomp: Adjust tests for using both STRICT and FILTER

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `mode_strict_upgrade_violation` test has a logic flaw that causes it to incorrectly pass even if the kernel mistakenly allows the forbidden syscall.
- [Medium] The patch drops test coverage for upgrading to strict mode via the `seccomp()` syscall.
--

commit 8c0b993065641d6398baa9878008c2ec51b13bef
Author: Jamie Hill-Daniel <clubby789@gmail.com>

selftest: seccomp: Adjust tests for using both STRICT and FILTER

Update tests to reflect that applying STRICT after FILTER is now possible.

> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 358b6c65e120e..17232591be590 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c

[ ... ]

> @@ -540,8 +540,34 @@ TEST(mode_filter_cannot_move_to_strict)
>  	ASSERT_EQ(0, ret);
>  
>  	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_STRICT, NULL, 0, 0);
> -	EXPECT_EQ(-1, ret);
> -	EXPECT_EQ(EINVAL, errno);
> +	ASSERT_EQ(0, ret);
> +	syscall(__NR_exit, 0);
> +}
> +
> +TEST_SIGNAL(mode_strict_upgrade_violation, SIGKILL)
> +{
> +	struct sock_filter filter[] = {
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> +	};
> +	struct sock_fprog prog = {
> +		.len = (unsigned short)ARRAY_SIZE(filter),
> +		.filter = filter,
> +	};
> +	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, &prog, 0, 0);
> +	ASSERT_EQ(0, ret);
> +
> +	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_STRICT, NULL, 0, 0);
> +	ASSERT_EQ(0, ret);
> +
> +	syscall(__NR_getpid);
> +	EXPECT_FALSE(true) {
> +		TH_LOG("Unreachable!");
> +	}
>  }

[Severity: Medium]
If the kernel mistakenly allows __NR_getpid, will the implicit return from this
test function mask the failure?

Returning from a kselftest function invokes the harness wrapper which calls
the standard library's _exit(), invoking the __NR_exit_group syscall. Because
__NR_exit_group is also forbidden in strict seccomp mode (only __NR_exit is
allowed), the kernel kills the test process with SIGKILL.

Since the parent harness expects a SIGKILL (due to the TEST_SIGNAL macro), it
matches the expected signal and overrides the test state to PASSED, hiding the
EXPECT_FALSE failure.

Should we explicitly call syscall(__NR_exit, 1); after the assertion fails to
ensure the test exits normally and the harness correctly fails it?

[ ... ]

> @@ -2376,42 +2402,6 @@ TEST(seccomp_syscall)
>  	}
>  }
>  
> -TEST(seccomp_syscall_mode_lock)
> -{
> -	struct sock_filter filter[] = {
> -		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> -	};
> -	struct sock_fprog prog = {
> -		.len = (unsigned short)ARRAY_SIZE(filter),
> -		.filter = filter,
> -	};
> -	long ret;
> -
> -	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0);
> -	ASSERT_EQ(0, ret) {
> -		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> -	}
> -
> -	ret = seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog);
> -	ASSERT_NE(ENOSYS, errno) {
> -		TH_LOG("Kernel does not support seccomp syscall!");
> -	}
> -	EXPECT_EQ(0, ret) {
> -		TH_LOG("Could not install filter!");
> -	}
> -
> -	/* Make sure neither entry point will switch to strict. */
> -	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_STRICT, 0, 0, 0);
> -	EXPECT_EQ(EINVAL, errno) {
> -		TH_LOG("Switched to mode strict!");
> -	}
> -
> -	ret = seccomp(SECCOMP_SET_MODE_STRICT, 0, NULL);
> -	EXPECT_EQ(EINVAL, errno) {
> -		TH_LOG("Switched to mode strict!");
> -	}
> -}

[Severity: Medium]
Does dropping this test also drop test coverage for upgrading to strict mode
via the seccomp() syscall?

The new replacement tests (mode_strict_upgrade and
mode_strict_upgrade_violation) only validate the prctl() interface, leaving the
seccomp() syscall path untested for the newly allowed transition from
SECCOMP_MODE_FILTER to SECCOMP_MODE_STRICT.

Could we add tests to ensure the seccomp() syscall correctly enforces this ABI?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-seccomp-combined-v2-0-41687a943ae0@hill-daniel.co.uk?part=3