Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown

On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:

Add the following simple testcases:

1. chmod/fchmod: remove S_IWUSR and restore S_IWUSR with or without

restriction. 2. chown/fchown: set original uid and gid with or without restriction, because chown needs CAP_CHOWN and testcase framework don't have this capability, setting original uid and gid is ok to cover landlock function.

Signed-off-by: Xiu Jianfeng xiujianfeng@huawei.com

tools/testing/selftests/landlock/fs_test.c | 228 +++++++++++++++++++++ 1 file changed, 228 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 5b55b93b5570..f47b4ccd2b26 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -59,6 +59,9 @@ static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";

static const char dir_s3d1[] = TMP_DIR "/s3d1"; static const char file1_s3d1[] = TMP_DIR "/s3d1/f1"; +static const char file2_s3d1[] = TMP_DIR "/s3d1/f2"; +static const char file3_s3d1[] = TMP_DIR "/s3d1/f3";

/* dir_s3d2 is a mount point. */ static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2"; static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; @@ -211,6 +214,8 @@ static void create_layout1(struct __test_metadata *const _metadata) create_file(_metadata, file2_s2d3);

create_file(_metadata, file1_s3d1);

• create_file(_metadata, file2_s3d1);
• create_file(_metadata, file3_s3d1); create_directory(_metadata, dir_s3d2); set_cap(_metadata, CAP_SYS_ADMIN); ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));

@@ -234,6 +239,8 @@ static void remove_layout1(struct __test_metadata *const _metadata) EXPECT_EQ(0, remove_path(file1_s2d1));

EXPECT_EQ(0, remove_path(file1_s3d1));

• EXPECT_EQ(0, remove_path(file2_s3d1));
• EXPECT_EQ(0, remove_path(file3_s3d1)); EXPECT_EQ(0, remove_path(dir_s3d3)); set_cap(_metadata, CAP_SYS_ADMIN); umount(dir_s3d2);

@@ -3272,6 +3279,227 @@ TEST_F_FORK(layout1, truncate) EXPECT_EQ(0, test_creat(file_in_dir_w)); }

+static int test_chmod(const char *path)

Nitpicks: - const char *const path - short documentation? :)

+{

• int ret;
• struct stat st;
• mode_t mode;
• ret = stat(path, &st);
• if (ret < 0)

• return errno;
  

• /* save original mode in order to restore */
• mode = st.st_mode & 0777;
• /* remove S_IWUSR */
• ret = chmod(path, mode & ~0200);
• if (ret < 0)

• return errno;
  

• ret = stat(path, &st);
• if (ret < 0)

• return errno;
  

• /* check if still has S_IWUSR */
• if (st.st_mode & 0200)

• return -EFAULT;
  

• /* restore the original mode */
• ret = chmod(path, mode);
• if (ret < 0)

• return errno;
  

• return 0;

+}

I would argue this can be simpler, with the following reasoning:

- Does the file have the right mode after chmod()?

I claim that fs_test should care only about the question of whether EACCES is returned or not. If fs_test were to also check for the side effects of these operations, it would eventually contain tests for the full file system API, not just for Landlock. That seems out of scope :)

- Undoing the chmod() operation

I'm not sure whether it's worth the effort to restore the exact state before that function returns. As long as the flags suffice to remove the test directory at the end, it probably doesn't matter much what exact mode they have?

I think this could just be

if (chmod(path, mode) < 0) return errno; return 0

and it would be a bit simpler to understand :)

The same argument applies also to the other test_...() functions.

+static int test_fchmod(const char *path)

I initially took the same approach for test_ftruncate() but eventually settled on using an approach where the file is open()ed before restricting the thread with Landlock. This eliminates the potential confusion where test_ftruncate() returns an error but the caller can't distinguish whether the error is from open() or from ftruncate(). It also makes fchmod testable even in scenarios where the file cannot be opened because of missing Landlock rights.

+{

• int ret, fd;
• struct stat st;
• mode_t mode;
• ret = stat(path, &st);
• if (ret < 0)

• return errno;
  

• /* save original mode in order to restore */
• mode = st.st_mode & 0777;
• fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
• if (fd < 0)

• return errno;
  

• /* remove S_IWUSR */
• ret = fchmod(fd, mode & ~0200);
• if (ret < 0)

• goto err;
  

• ret = stat(path, &st);
• if (ret < 0)

• goto err;
  

• /* check if still has S_IWUSR */
• if (st.st_mode & 0200) {

• ret = -1;
  

• errno = -EFAULT;
  

• goto err;
  

• }
• /* restore the original mode */
• ret = fchmod(fd, mode);

+err:

• if (close(fd) < 0)

• return errno;
  

• return ret ? errno : 0;

+}

+static int test_chown(const char *path) +{

• int ret;
• struct stat st;
• ret = stat(path, &st);
• if (ret < 0)

• return errno;
  

• /*

• * chown needs CAP_CHOWN to modify uid and/or gid, however
  

• * there is no such capability when the testcases framework
  

• * setup, so just chown to original uid/gid, which can also
  

• * cover the function in landlock.
  

• */
  

• ret = chown(path, st.st_uid, st.st_gid);
• if (ret < 0)

• return errno;
  

• return 0;

+}

+static int test_fchown(const char *path) +{

• int ret, fd;
• struct stat st;
• ret = stat(path, &st);
• if (ret < 0)

• return errno;
  

• fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
• if (fd < 0)

• return errno;
  

• /*

• * fchown needs CAP_CHOWN to modify uid and/or gid, however
  

• * there is no such capability when the testcases framework
  

• * setup, so just fchown to original uid/gid, which can also
  

• * cover the function in landlock.
  

• */
  

• ret = fchown(fd, st.st_uid, st.st_gid);
• if (close(fd) < 0)

• return errno;
  

• return ret ? errno : 0;

+}

+TEST_F_FORK(layout1, unhandled_chmod) +{

• const struct rule rules[] = {

• {
  

• 	.path = file2_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE,
  

• },
  

• {
  

• 	.path = file3_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE,
  

• },
  

• {},
  

• };
• const int ruleset_fd =

• create_ruleset(_metadata, ACCESS_RW, rules);
  

• ASSERT_LE(0, ruleset_fd);
• enforce_ruleset(_metadata, ruleset_fd);
• ASSERT_EQ(0, close(ruleset_fd));
• ASSERT_EQ(0, test_chmod(file2_s3d1));
• ASSERT_EQ(0, test_fchmod(file2_s3d1));
• ASSERT_EQ(0, test_chmod(file3_s3d1));
• ASSERT_EQ(0, test_chmod(dir_s3d1));

*optional* because the existing tests are already inconsistent about it

These four ASSERT_EQ() calls are independent scenarios and could be done with EXPECT_EQ(), which would be more in line with the approach that this test framework takes. (Same for the other tests below)

Compare previous discussion at: https://lore.kernel.org/all/Yvd3+fy+mDBop+YA@nuc/

+}

+TEST_F_FORK(layout1, chmod) +{

• const struct rule rules[] = {

• {
  

• 	.path = file2_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE |
  

• 		  LANDLOCK_ACCESS_FS_CHMOD,
  

• },
  

• {
  

• 	.path = file3_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE,
  

• },
  

• {},
  

• };
• const int ruleset_fd =

• create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
  

• ASSERT_LE(0, ruleset_fd);
• enforce_ruleset(_metadata, ruleset_fd);
• ASSERT_EQ(0, close(ruleset_fd));
• ASSERT_EQ(0, test_chmod(file2_s3d1));
• ASSERT_EQ(0, test_fchmod(file2_s3d1));
• ASSERT_EQ(EACCES, test_chmod(file3_s3d1));
• ASSERT_EQ(EACCES, test_chmod(dir_s3d1));

+}

+TEST_F_FORK(layout1, no_chown)

"unhandled_chown" to be consistent with the other one above?

+{

• const struct rule rules[] = {

• {
  

• 	.path = file2_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE,
  

• },
  

• {
  

• 	.path = file3_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE,
  

• },
  

• {},
  

• };
• const int ruleset_fd =

• create_ruleset(_metadata, ACCESS_RW, rules);
  

• ASSERT_LE(0, ruleset_fd);
• enforce_ruleset(_metadata, ruleset_fd);
• ASSERT_EQ(0, close(ruleset_fd));
• ASSERT_EQ(0, test_chown(file2_s3d1));
• ASSERT_EQ(0, test_fchown(file2_s3d1));
• ASSERT_EQ(0, test_chown(file3_s3d1));
• ASSERT_EQ(0, test_chown(dir_s3d1));

+}

+TEST_F_FORK(layout1, chown) +{

• const struct rule rules[] = {

• {
  

• 	.path = file2_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE |
  

• 		  LANDLOCK_ACCESS_FS_CHOWN,
  

It might be useful to also check a scenario where the chown right is granted on a directory (and as a consequence, both the directory itself as well as its contents can be chowned)? (Same for chmod)

• },
  

• {
  

• 	.path = file3_s3d1,
  

• 	.access = LANDLOCK_ACCESS_FS_READ_FILE |
  

• 		  LANDLOCK_ACCESS_FS_WRITE_FILE,
  

• },
  

• {},
  

• };
• const int ruleset_fd =

• create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHOWN, rules);
  

• ASSERT_LE(0, ruleset_fd);
• enforce_ruleset(_metadata, ruleset_fd);
• ASSERT_EQ(0, close(ruleset_fd));
• ASSERT_EQ(0, test_chown(file2_s3d1));
• ASSERT_EQ(0, test_fchown(file2_s3d1));
• ASSERT_EQ(EACCES, test_chown(file3_s3d1));
• ASSERT_EQ(EACCES, test_chown(dir_s3d1));

+}

/* clang-format off */ FIXTURE(layout1_bind) {}; /* clang-format on */ -- 2.17.1

--