Re: [PATCH 2/2] selftests/safesetid: test GID policy with mismatched real IDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: false test failure on older kernels due to strict prctl() check
- [Low] selftests: build failure on older glibc due to SYS_pidfd_open dependency
--

commit dbf0ec68f44394083debd3f938ca7365c6cad4f1
Author: Sung Byeongchan <tjdqudcks0424@naver.com>

selftests/safesetid: test GID policy with mismatched real IDs

This commit adds child-process cases to the SafeSetID selftests that use
mismatched real IDs to properly distinguish the source of a GID policy lookup.
It verifies that allowed and forbidden transitions are handled correctly using
both setresgid() and setgroups().

> diff --git a/tools/testing/selftests/safesetid/safesetid-gid-policy-test.c b/tools/testing/selftests/safesetid/safesetid-gid-policy-test.c
> new file mode 100644
> index 0000000000000..873d65627c494
> --- /dev/null
> +++ b/tools/testing/selftests/safesetid/safesetid-gid-policy-test.c
[ ... ]
> +static int check_child_credentials(void)
> +{
[ ... ]
> +	if (effective != expected || permitted != expected || inheritable) {
> +		fprintf(stderr, "child does not hold only CAP_SETGID\n");
> +		return -1;
> +	}
> +	for (i = 0; i <= CAP_LAST_CAP; i++) {
> +		if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, i, 0, 0) != 0) {

[Severity: Medium]
Will this strict non-zero check cause the test to falsely fail on older
kernels?

If the selftest is compiled with newer kernel headers (where CAP_LAST_CAP is
higher) but executed on an older kernel, prctl(PR_CAP_AMBIENT_IS_SET) will
return -1 (-EINVAL) for newly defined capabilities that the older kernel does
not support.

Since the test checks for != 0 instead of exactly == 1 (or explicitly
ignoring -1), the -EINVAL error return is treated as an unexpected ambient
capability.

Could this be updated to check for == 1 or ignore -1 to prevent false
failures?

> +			fprintf(stderr, "unexpected ambient capability %d\n", i);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
[ ... ]
> +static int run_case(const char *name, enum test_operation operation,
> +		    gid_t target, bool expect_kill)
> +{
> +	struct pollfd pollfd = {};
> +	int status;
> +	int pidfd;
> +	pid_t child;
> +
> +	child = fork();
> +	if (child < 0) {
> +		perror("fork");
> +		return -1;
> +	}
> +	if (!child)
> +		run_child(operation, target);
> +
> +	pidfd = syscall(SYS_pidfd_open, child, 0);

[Severity: Low]
Does this rely on host glibc syscall definitions instead of the kernel-provided
headers?

Older glibc versions (prior to 2.36) do not define SYS_pidfd_open, which will
result in a compilation error when building the selftests on an older host.

Using the kernel-provided __NR_pidfd_open is the standard approach in
selftests to maintain backwards compatibility and independence from the host
libc's system call knowledge. Could __NR_pidfd_open be used here instead?

> +	if (pidfd < 0) {
> +		perror("pidfd_open");
> +		kill(child, SIGKILL);
> +		waitpid(child, &status, 0);
> +		return -1;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791023194.git.tjdqudcks0424@naver.com?part=2