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 <[email protected]> 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/[email protected]?part=16
