Please use "selftests/landlock:" as subject prefix and without a final dot.


On 07/07/2022 22:06, Günther Noack wrote:

These tests exercise the following scenarios:

* File with Read, Write, Truncate rights.

Should we use a capital for access right names or does it come from Go? ;)

* File with Read, Write rights.
* File with Truncate rights.
* File with no rights.
* Directory with Truncate rights.

For each of the scenarios, both truncate() and the open() +
ftruncate() syscalls get exercised and their results checked.

In particular, the test demonstrates that opening a file for writing
is not enough to call truncate().

Looks good! According to my previous comment, O_TRUNC should be tested 
if it is checked by the kernel.

Please move this test after the proc_pipe one.

+{
+	const struct rule rules[] = {

You can add a first layer of rules to check truncate and ftruncate with 
a ruleset not handling LANDLOCK_ACCESS_FS_TRUNCATE.

+		{
+			.path = file1_s1d1,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file2_s1d2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = file1_s1d2,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},

Move this entry before file2_s1d2 to keep the paths sorted and make this 
easier to read. You can change the access rights per path to also keep 
their ordering though.

+		{
+			.path = dir_s2d3,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		// Implicitly: No access rights for file2_s1d1.

Comment to move after the use of file1_s1d1.

+		{},
+	};
+	const int ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, rules);

Don't use ACCESS_ALL because it will change over time and we want tests 
to be deterministic. You can use rules[0].access instead.

+	int reg_fd;
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Read, write and truncate permissions => truncate and ftruncate work. */

It would be nice to have consistent comments such as: "Checks read, 
write and truncate access rights: truncate and ftruncate work."

+	reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	EXPECT_EQ(0, ftruncate(reg_fd, 10));

You should not use EXPECT but ASSERT here. I use EXPECT when an error 
could block a test or when it could stop a cleanup (i.e. teardown).

+	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
+	ASSERT_EQ(0, close(reg_fd));
+
+	EXPECT_EQ(0, truncate(file1_s1d1, 10));
+	EXPECT_EQ(0, truncate64(file1_s1d1, 20));
+
+	/* Just read and write permissions => no truncate variant works. */
+	reg_fd = open(file2_s1d2, O_RDWR | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	EXPECT_EQ(-1, ftruncate(reg_fd, 10));
+	EXPECT_EQ(EACCES, errno);
+	EXPECT_EQ(-1, ftruncate64(reg_fd, 20));
+	EXPECT_EQ(EACCES, errno);
+	ASSERT_EQ(0, close(reg_fd));
+
+	EXPECT_EQ(-1, truncate(file2_s1d2, 10));
+	EXPECT_EQ(EACCES, errno);
+	EXPECT_EQ(-1, truncate64(file2_s1d2, 20));
+	EXPECT_EQ(EACCES, errno);
+
+	/* Just truncate permissions => truncate(64) works, but can't open file. */
+	ASSERT_EQ(-1, open(file1_s1d2, O_RDWR | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+
+	EXPECT_EQ(0, truncate(file1_s1d2, 10));
+	EXPECT_EQ(0, truncate64(file1_s1d2, 20));
+
+	/* Just truncate permission on directory => truncate(64) works, but can't open file. */
+	ASSERT_EQ(-1, open(file1_s2d3, O_RDWR | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+
+	EXPECT_EQ(0, truncate(file1_s2d3, 10));
+	EXPECT_EQ(0, truncate64(file1_s2d3, 20));
+
+	/* No permissions => Neither truncate nor ftruncate work. */
+	ASSERT_EQ(-1, open(file2_s1d1, O_RDWR | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, truncate(file2_s1d1, 10));
+	EXPECT_EQ(EACCES, errno);
+	EXPECT_EQ(-1, truncate64(file2_s1d1, 20));
+	EXPECT_EQ(EACCES, errno);

These tests are good!

+}
+
  static void
  reparent_exdev_layers_enforce1(struct __test_metadata *const _metadata)
  {