Re: [PATCH v3 -next 16/18] selftests/landlock: Add tests for metadata access rights

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/landlock: hardcoded GID 0 in chown metadata tests fails for non-root
- [Low] selftests/landlock: duplicate linux/fs.h include before sys/mount.h
- [Low] selftests/landlock: missing MIPS offset in file_getattr/setattr fallback NRs
--

commit 76c1b6128ed1afd1dd4933dbcba43ffec623cd33
Author: Cai Xinchen <caixinchen1@huawei.com>

selftests/landlock: Add tests for metadata access rights

This commit adds selftests covering the LANDLOCK_ACCESS_FS_READ_METADATA and
LANDLOCK_ACCESS_FS_WRITE_METADATA access rights across chmod, chown,
utimensat, xattr, stat, fileattr, open-time semantics, and overlayfs. It also
updates the ABI version and access mask definitions in the Landlock test
suite.

> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index f725882fa4a78..1e819ba952ac6 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -12,6 +12,7 @@
>  #include <fcntl.h>
>  #include <libgen.h>
>  #include <linux/fiemap.h>
> +#include <linux/fs.h>
>  #include <linux/landlock.h>
>  #include <linux/magic.h>
>  #include <sched.h>

[Severity: Low]
Could adding this include of linux/fs.h before sys/mount.h break compilation
on toolchains where glibc headers conflict with UAPI linux/mount.h?

Further down in tools/testing/selftests/landlock/fs_test.c, linux/fs.h is
already included after sys/mount.h and sys/vfs.h with a comment noting the
ordering requirement:

    /*
     * Intentionally included last to work around header conflict.
     * See https://sourceware.org/glibc/wiki/Synchronizing_Headers.
     */
    #include <linux/fs.h>
    #include <linux/mount.h>

Because UAPI linux/fs.h includes linux/mount.h when __KERNEL__ is not
defined, including linux/fs.h at the top of the file pulls in linux/mount.h
prior to sys/mount.h and makes the deferred include a no-op.

[ ... ]

> @@ -10916,4 +10921,918 @@ TEST_F(trace_layout1, check_rule_fs_empty_grant)

[ ... ]

> +TEST_F_FORK(layout1, unhandled_metadata)
> +{
> +	int file_fd;
> +	int ruleset_fd;
> +	const char *const file1 = file1_s1d1;
> +	const char *const file2 = file2_s1d1;
> +	const char *const dir1 = dir_s1d1;
> +	struct stat statbuf;
> +	const struct rule rules[] = {
> +		{
> +			.path = file1,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = dir1,
> +			.access = ACCESS_RW,
> +		},
> +		{},
> +	};
> +
> +	/*
> +	 * READ_METADATA and WRITE_METADATA are not handled, so metadata
> +	 * access should be allowed.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	file_fd = open(file1, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_fd);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	EXPECT_EQ(0, test_chmod(file1, 0644));
> +	EXPECT_EQ(0, test_fchmod(file_fd, 0644));
> +	EXPECT_EQ(0, test_chmod(file2, 0400));
> +	EXPECT_EQ(0, test_chmod(dir1, 0700));
> +
> +	EXPECT_EQ(0, test_chown(file1, -1, 0));
> +	EXPECT_EQ(0, test_fchown(file_fd, -1, 0));
> +	EXPECT_EQ(0, test_lchown(file1, -1, 0));

[Severity: Medium]
What happens in unhandled_metadata (as well as in write_metadata,
metadata_open_time, and metadata_transfer) when the test binary runs with a
non-zero GID or inside a user namespace where GID 0 is unmapped?

Landlock selftests support running as a non-root user with capabilities via
SECBIT_NOROOT in _init_caps(). During fixture setup,
FIXTURE_SETUP(layout1) -> prepare_layout_opt() -> disable_caps() clears
effective capabilities and drops CAP_CHOWN from the permitted set, creating
test files owned by geteuid():getegid().

When getegid() is non-zero and the caller is not a member of group 0,
chown_common() -> notify_change() -> setattr_prepare() -> chgrp_ok() rejects
changing the group to hardcoded GID 0 with -EPERM because CAP_CHOWN is not
held, causing the positive EXPECT_EQ(0, ...) assertions to fail. Similarly,
if GID 0 is unmapped in the current user namespace, chown_common() fails
setattr_vfsgid() with -EINVAL before reaching security_path_setattr(),
causing the EXPECT_EQ(EACCES, ...) checks to fail as well.

Would passing getegid() instead of 0 to test_chown(), test_fchown(), and
test_lchown() avoid depending on the caller's GID while still setting
ATTR_GID in chown_common() and exercising hook_path_setattr()?

[ ... ]

> +#ifndef __NR_file_getattr
> +#if defined(__alpha__)
> +#define __NR_file_getattr 578
> +#else
> +#define __NR_file_getattr 468
> +#endif
> +#endif
> +#ifndef __NR_file_setattr
> +#if defined(__alpha__)
> +#define __NR_file_setattr 579
> +#else
> +#define __NR_file_setattr 469
> +#endif
> +#endif

[Severity: Low]
When building against older UAPI headers on MIPS, won't these fallback
definitions for __NR_file_getattr and __NR_file_setattr evaluate to raw 468
and 469 without the MIPS ABI base offset (__NR_Linux + 468 and
__NR_Linux + 469, where __NR_Linux is 4000 for o32, 5000 for n64, and 6000
for n32), causing test_file_getattr() and test_file_setattr() to fail with
ENOSYS?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027121151.1887650-1-caixinchen1@huawei.com?part=16