Re: [PATCH v5 5/8] selftests/landlock: Add namespace restriction tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests: Build failure due to missing AT_RECURSIVE macro definition
- [Low] selftests: Missing clear_cap() for CAP_SYS_CHROOT in combined_cap_ns test
- [Low] selftests: is_in_init_user_ns() misidentifies user namespaces in unprivileged containers
--

commit aa1731ea4300b766831441aec03fce370fdad94e
Author: Mickaël Salaün <mic@digikod.net>

selftests/landlock: Add namespace restriction tests

Add tests covering LANDLOCK_PERMISSION_NAMESPACE_USE (namespace creation
via unshare/clone and namespace entry via setns) and its interaction
with LANDLOCK_PERMISSION_CAPABILITY_USE.

> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -130,6 +130,29 @@ static void __maybe_unused clear_ambient_cap(
>  	EXPECT_EQ(0, cap_get_ambient(cap));
>  }
>  
> +/*
> + * Returns true if the current process is in the initial user namespace.
> + * Compares the readlink targets of /proc/self/ns/user and /proc/1/ns/user.
> + */
> +static bool __maybe_unused is_in_init_user_ns(void)
> +{
[ ... ]
> +	if (self_len != init_len)
> +		return false;
> +
> +	return memcmp(self_buf, init_buf, self_len) == 0;
> +}

[Severity: Low]
Is it possible for this check to misidentify the initial user namespace when
running in an unprivileged container?

In a container with its own PID namespace, PID 1 refers to the container's
init, which shares the container's user namespace. This causes the function
to incorrectly return true.

This can result in the test proceeding and attempting host-only operations:

> diff --git a/tools/testing/selftests/landlock/ns_test.c b/tools/testing/selftests/landlock/ns_test.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/ns_test.c
[ ... ]
> +FIXTURE_SETUP(ns_audit)
> +{
> +	ASSERT_TRUE(is_in_init_user_ns());
> +
> +	disable_caps(_metadata);
> +
> +	set_cap(_metadata, CAP_AUDIT_CONTROL);
> +	self->audit_fd = audit_init_with_exe_filter(&self->audit_filter);
> +	EXPECT_LE(0, self->audit_fd);

If the environment is misidentified, audit_init_with_exe_filter() will fail
with a misleading -EPERM error instead of explicitly failing the environment
check or skipping the test cleanly.

[ ... ]
> +TEST(combined_cap_ns)
> +{
[ ... ]
> +	/* CAP_SYS_ADMIN use allowed by capability rule. */
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	EXPECT_EQ(0, sethostname("test", 4));
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	/* CAP_SYS_CHROOT denied (not in allowed capability rules). */
> +	set_cap(_metadata, CAP_SYS_CHROOT);
> +	EXPECT_EQ(-1, chroot("/"));
> +	EXPECT_EQ(EPERM, errno);

[Severity: Low]
Should clear_cap(_metadata, CAP_SYS_CHROOT) be called after this check to
avoid leaving the capability unintentionally active for the remainder of the
test?

[ ... ]
> +TEST_F(ns_mount_fd, open_tree_clone)
> +{
[ ... ]
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	fd = sys_open_tree(AT_FDCWD, "/",
> +			   OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC | AT_RECURSIVE);

[Severity: Medium]
Will this fail to compile on older distributions due to a missing
AT_RECURSIVE definition?

sys_open_tree() is called with the AT_RECURSIVE flag, but the file includes
<fcntl.h> instead of the kernel's <linux/fcntl.h>. Older glibc versions do
not define AT_RECURSIVE in their userspace headers.

Without an explicit fallback definition or an inclusion of <linux/fcntl.h>,
compiling these selftests could result in an 'AT_RECURSIVE undeclared' error.

This also applies to the usage in open_tree_namespace:

> +TEST_F(ns_mount_fd, open_tree_namespace)
> +{
[ ... ]
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	fd = sys_open_tree(AT_FDCWD, "/",
> +			   OPEN_TREE_NAMESPACE | OPEN_TREE_CLOEXEC |
> +				   AT_RECURSIVE);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007100255.1333386-1-mic@digikod.net?part=5