diff mbox series

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

Message ID 20220822114701.26975-4-xiujianfeng@huawei.com (mailing list archive)
State New
Headers show
Series landlock: add chmod and chown support | expand

Commit Message

xiujianfeng Aug. 22, 2022, 11:46 a.m. UTC
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(+)

Comments

Günther Noack Aug. 22, 2022, 6:53 p.m. UTC | #1
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
>

--
Günther Noack Aug. 22, 2022, 7:26 p.m. UTC | #2
On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
> +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));
> +}

I missed it in the previous mail:

There are also the chown variants lchown() and fchownat(), as well as
the chmod variant fchmodat(), which might be interesting to test,
especially the symlink scenarios.

fchmodat() has a AT_SYMLINK_NOFOLLOW flag which does the chmod
equivalent to lchown().

--
Mickaël Salaün Aug. 22, 2022, 9:28 p.m. UTC | #3
Agree with Günther's review. :)


On 22/08/2022 20:53, Günther Noack wrote:
> 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
>>
> 
> --
xiujianfeng Aug. 24, 2022, 8:27 a.m. UTC | #4
Hi,

Thanks for your review, all comments are helpfull, will do in v2.

在 2022/8/23 2:53, Günther Noack 写道:
> 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
>>
> 
> --
> .
>
xiujianfeng Aug. 27, 2022, 11:14 a.m. UTC | #5
Hi,

在 2022/8/23 3:26, Günther Noack 写道:
> On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
>> +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));
>> +}
> 
> I missed it in the previous mail:
> 
> There are also the chown variants lchown() and fchownat(), as well as
> the chmod variant fchmodat(), which might be interesting to test,
> especially the symlink scenarios.
> 
> fchmodat() has a AT_SYMLINK_NOFOLLOW flag which does the chmod
> equivalent to lchown().
> 

man fchmodat shows as follows:
...
AT_SYMLINK_NOFOLLOW
If pathname is a symbolic link, do not dereference it: instead operate 
on the link itself. This flag is not currently implemented.
...

so I suppose this can not be test. Please correct me if I am wrong.
thanks.

> --
> .
>
Günther Noack Aug. 27, 2022, 4:57 p.m. UTC | #6
On Sat, Aug 27, 2022 at 07:14:30PM +0800, xiujianfeng wrote:
> Hi,
>
> 在 2022/8/23 3:26, Günther Noack 写道:
> > On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
> > > +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));
> > > +}
> >
> > I missed it in the previous mail:
> >
> > There are also the chown variants lchown() and fchownat(), as well as
> > the chmod variant fchmodat(), which might be interesting to test,
> > especially the symlink scenarios.
> >
> > fchmodat() has a AT_SYMLINK_NOFOLLOW flag which does the chmod
> > equivalent to lchown().
> >
>
> man fchmodat shows as follows:
> ...
> AT_SYMLINK_NOFOLLOW
> If pathname is a symbolic link, do not dereference it: instead operate on
> the link itself. This flag is not currently implemented.
> ...
>
> so I suppose this can not be test. Please correct me if I am wrong.

Good point - I didn't realize that AT_SYMLINK_NOFOLLOW was not
implemented for fchmodat. In that case, this only applies to lchown.

—Günther

--
diff mbox series

Patch

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)
+{
+	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;
+}
+
+static int test_fchmod(const char *path)
+{
+	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));
+}
+
+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)
+{
+	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,
+		},
+		{
+			.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 */