[PATCH v4 0/4] landlock: truncate support

[PATCH v4 0/4] landlock: truncate support

[Günther Noack]

The goal of these patches is to work towards a more complete coverage
of file system operations that are restrictable with Landlock.

The known set of currently unsupported file system operations in
Landlock is described at [1]. Out of the operations listed there,
truncate is the only one that modifies file contents, so these patches
should make it possible to prevent the direct modification of file
contents with Landlock.

The patch introduces the truncation restriction feature as an
additional bit in the access_mask_t bitmap, in line with the existing
supported operations.

The truncation flag covers both the truncate(2) and ftruncate(2)
families of syscalls, as well as open(2) with the O_TRUNC flag.
This includes usages of creat() in the case where existing regular
files are overwritten.

Apart from Landlock, file truncation can also be restricted using
seccomp-bpf, but it is more difficult to use (requires BPF, requires
keeping up-to-date syscall lists) and it is not configurable by file
hierarchy, as Landlock is. The simplicity and flexibility of the
Landlock approach makes it worthwhile adding.

While it's possible to use the "write file" and "truncate" rights
independent of each other, it simplifies the mental model for
userspace callers to always use them together.

Specifically, the following behaviours might be surprising for users
when using these independently:

 * The commonly creat() syscall requires the truncate right when
   overwriting existing files, as it is equivalent to open(2) with
   O_TRUNC|O_CREAT|O_WRONLY.
 * The "write file" right is not always required to truncate a file,
   even through the open(2) syscall (when using O_RDONLY|O_TRUNC).

Nevertheless, keeping the two flags separate is the correct approach
to guarantee backwards compatibility for existing Landlock users.

These patches are based on version 5.19.

Best regards,
Günther

[1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags

History:

v4:
 * Documentation
   * Clarify wording and syntax as discussed in review.
   * Use a less confusing error message in the example.
 * selftests:
   * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
     (This is an intentionally uncommon error code, so that the source
     of the error is clear and the test can distinguish test setup
     failures from failures in the actual system call under test.)
 * samples/Documentation:
   * Use additional clarifying comments in the kernel backwards
     compatibility logic.

v3:
 * selftests:
   * Explicitly test ftruncate with readonly file descriptors
     (returns EINVAL).
   * Extract test_ftruncate, test_truncate, test_creat helpers,
     which simplified the previously mixed usage of EXPECT/ASSERT.
   * Test creat() behaviour as part of the big truncation test.
   * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
     This simplifies the tests a bit. The kernel implementations are the
     same as for truncate(2) and ftruncate(2), so there is little benefit
     from testing them exhaustively. (We aren't testing all open(2)
     variants either.)
 * samples/landlock/sandboxer.c:
   * Use switch() to implement best effort mode.
 * Documentation:
   * Give more background on surprising truncation behaviour.
   * Use switch() in the example too, to stay in-line with the sample tool.
   * Small fixes in header file to address previous comments.
* misc:
  * Fix some typos and const usages.

v2:
 * Documentation: Mention the truncation flag where needed.
 * Documentation: Point out connection between truncation and file writing.
 * samples: Add file truncation to the landlock/sandboxer.c sample tool.
 * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
 * selftests: Exercise truncation syscalls when the truncate right
   is not handled by Landlock.

Previous versions:
v1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
v2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
v3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/

Günther Noack (4):
  landlock: Support file truncation
  selftests/landlock: Selftests for file truncation support
  samples/landlock: Extend sample tool to support
    LANDLOCK_ACCESS_FS_TRUNCATE
  landlock: Document Landlock's file truncation support

 Documentation/userspace-api/landlock.rst     |  52 ++++-
 include/uapi/linux/landlock.h                |  17 +-
 samples/landlock/sandboxer.c                 |  23 +-
 security/landlock/fs.c                       |   9 +-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 226 ++++++++++++++++++-
 8 files changed, 305 insertions(+), 28 deletions(-)


base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
--
2.37.2

[PATCH v4 1/4] landlock: Support file truncation

[Günther Noack]

Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.

This flag hooks into the path_truncate LSM hook and covers file
truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
well as creat().

This change also increments the Landlock ABI version, updates
corresponding selftests, and includes minor documentation changes to
document the flag.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 Documentation/userspace-api/landlock.rst     | 10 ++++++++++
 include/uapi/linux/landlock.h                | 17 ++++++++++++-----
 security/landlock/fs.c                       |  9 ++++++++-
 security/landlock/limits.h                   |  2 +-
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  7 ++++---
 7 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index b8ea59493964..6648e59fabe7 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -380,6 +380,16 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
 Previous limitations
 ====================
 
+File truncation (ABI < 3)
+-------------------------
+
+File truncation could not be denied before the third Landlock ABI, so it is
+always allowed when using a kernel that only supports the first or second ABI.
+
+Starting with the Landlock ABI version 3, it is now possible to securely
+control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access
+right.
+
 File renaming and linking (ABI 1)
 ---------------------------------
 
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 23df4e0e8ace..a2fef267bf34 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
  * A file can only receive these access rights:
  *
  * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
- * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
+ * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
+ *   you might additionally need the `LANDLOCK_ACCESS_FS_TRUNCATE` right in
+ *   order to overwrite files with :manpage:`open(2)` using `O_TRUNC` or
+ *   :manpage:`creat(2)`.
  * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
+ * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation APIs
+ *   like :manpage:`truncate(2)`, :manpage:`ftruncate(2)`, or
+ *   :manpage:`open(2)` with `O_TRUNC` or :manpage:`creat(2)`. This access right
+ *   is available since the third version of the Landlock ABI.
  *
  * A directory can receive access rights related to files or directories.  The
  * following access right is applied to the directory itself, and the
@@ -139,10 +146,9 @@ struct landlock_path_beneath_attr {
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
- *   :manpage:`truncate(2)`, :manpage:`stat(2)`, :manpage:`flock(2)`,
- *   :manpage:`chmod(2)`, :manpage:`chown(2)`, :manpage:`setxattr(2)`,
- *   :manpage:`utime(2)`, :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
- *   :manpage:`access(2)`.
+ *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
+ *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
+ *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -160,6 +166,7 @@ struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
+#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index ec5a6247cd3e..c57f581a9cd5 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 /* clang-format on */
 
 /*
@@ -1140,6 +1141,11 @@ static int hook_path_rmdir(const struct path *const dir,
 	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
 }
 
+static int hook_path_truncate(const struct path *const path)
+{
+	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
+}
+
 /* File hooks */
 
 static inline access_mask_t get_file_access(const struct file *const file)
@@ -1192,6 +1198,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
 	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
 	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
+	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
 
 	LSM_HOOK_INIT(file_open, hook_file_open),
 };
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index b54184ab9439..82288f0e9e5e 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_REFER
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 735a0865ea11..f4d6fc7ed17f 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 2
+#define LANDLOCK_ABI_VERSION 3
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index da9290817866..72cdae277b02 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(2, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21a2ce8fa739..cb77eaa01c91 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -399,9 +399,10 @@ TEST_F_FORK(layout1, inval)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
@@ -415,7 +416,7 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	ACCESS_LAST)
+	LANDLOCK_ACCESS_FS_REFER)
 
 /* clang-format on */
 
-- 
2.37.2

[PATCH v4 2/4] selftests/landlock: Selftests for file truncation support

[Günther Noack]

These tests exercise the following truncation operations:

* truncate() (truncate by path)
* ftruncate() (truncate by file descriptor)
* open with the O_TRUNC flag
* special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.

in the following scenarios:

* Files with read, write and truncate rights.
* Files with read and truncate rights.
* Files with the truncate right.
* Files without the truncate right.

In particular, the following scenarios are enforced with the test:

* The truncate right is required to use ftruncate,
  even when the thread already has the right to write to the file.
* open() with O_TRUNC requires the truncate right, if it truncates a file.
  open() already checks security_path_truncate() in this case,
  and it required no additional check in the Landlock LSM's file_open hook.
* creat() requires the truncate right
  when called with an existing filename.
* creat() does *not* require the truncate right
  when it's creating a new file.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 tools/testing/selftests/landlock/fs_test.c | 219 +++++++++++++++++++++
 1 file changed, 219 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index cb77eaa01c91..7a2ce6cd1a5a 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
 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";
 /* 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";
@@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
  * │           ├── f1
  * │           └── f2
  * └── s3d1
+ *     ├── f1
  *     └── s3d2
  *         └── s3d3
  */
@@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
 	create_file(_metadata, file1_s2d3);
 	create_file(_metadata, file2_s2d3);
 
+	create_file(_metadata, file1_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"));
@@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
 	EXPECT_EQ(0, remove_path(file1_s2d2));
 	EXPECT_EQ(0, remove_path(file1_s2d1));
 
+	EXPECT_EQ(0, remove_path(file1_s3d1));
 	EXPECT_EQ(0, remove_path(dir_s3d3));
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	umount(dir_s3d2);
@@ -3023,6 +3027,221 @@ TEST_F_FORK(layout1, proc_pipe)
 	ASSERT_EQ(0, close(pipe_fds[1]));
 }
 
+/*
+ * Opens the file and invokes ftruncate(2).
+ *
+ * Returns the errno of ftruncate if ftruncate() fails.
+ * Returns EBADFD if open() or close() fail (should not happen).
+ * Returns 0 if ftruncate(), open() and close() were successful.
+ */
+static int test_ftruncate(struct __test_metadata *const _metadata,
+			  const char *const path, int flags)
+{
+	int res, err, fd;
+
+	fd = open(path, flags | O_CLOEXEC);
+	if (fd < 0)
+		return EBADFD;
+
+	res = ftruncate(fd, 10);
+	err = errno;
+
+	if (close(fd) != 0)
+		return EBADFD;
+
+	if (res < 0)
+		return err;
+	return 0;
+}
+
+/* Invokes truncate(2) and returns the errno or 0. */
+static int test_truncate(const char *const path)
+{
+	if (truncate(path, 10) < 0)
+		return errno;
+	return 0;
+}
+
+/*
+ * Invokes creat(2) and returns its errno or 0.
+ * Closes the opened file descriptor on success.
+ * Returns EBADFD if close() returns an error (should not happen).
+ */
+static int test_creat(struct __test_metadata *const _metadata,
+		      const char *const path, mode_t mode)
+{
+	int fd = creat(path, mode);
+
+	if (fd < 0)
+		return errno;
+
+	if (close(fd) < 0)
+		return EBADFD;
+	return 0;
+}
+
+TEST_F_FORK(layout1, truncate)
+{
+	const char *const file_rwt = file1_s1d1;
+	const char *const file_rw = file2_s1d1;
+	const char *const file_rt = file1_s1d2;
+	const char *const file_t = file2_s1d2;
+	const char *const file_none = file1_s1d3;
+	const char *const dir_t = dir_s2d1;
+	const char *const file_in_dir_t = file1_s2d1;
+	const char *const dir_w = dir_s3d1;
+	const char *const file_in_dir_w = file1_s3d1;
+	const struct rule rules[] = {
+		{
+			.path = file_rwt,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file_rw,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = file_rt,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file_t,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		// Implicitly: No access rights for file_none.
+		{
+			.path = dir_t,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = dir_w,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
+			      LANDLOCK_ACCESS_FS_WRITE_FILE |
+			      LANDLOCK_ACCESS_FS_TRUNCATE;
+	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/*
+	 * Checks read, write and truncate rights: truncation works.
+	 *
+	 * Note: Independent of Landlock, ftruncate(2) on read-only
+	 * file descriptors never works.
+	 */
+	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
+	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
+	EXPECT_EQ(0, test_truncate(file_rwt));
+	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
+
+	/* Checks read and write rights: no truncate variant works. */
+	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
+	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
+	EXPECT_EQ(EACCES, test_truncate(file_rw));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
+
+	/*
+	 * Checks read and truncate rights: truncation works.
+	 *
+	 * Note: Files opened in O_RDONLY can get truncated as part of
+	 * the same operation.
+	 */
+	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
+	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
+	EXPECT_EQ(0, test_truncate(file_rt));
+
+	/* Checks truncate right: truncate works, but can't open file. */
+	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
+	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_truncate(file_t));
+
+	/* Checks "no rights" case: No form of truncation works. */
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_truncate(file_none));
+
+	/* Checks truncate right on directory: truncate works on contained files */
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_truncate(file_in_dir_t));
+
+	/*
+	 * Checks creat in dir_w: This requires the truncate right
+	 * when overwriting an existing file, but does not require it
+	 * when the file is new.
+	 */
+	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
+
+	ASSERT_EQ(0, unlink(file_in_dir_w));
+	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
+}
+
+/*
+ * Exercises file truncation when it's not restricted,
+ * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
+ */
+TEST_F_FORK(layout1, truncate_unhandled)
+{
+	const char *const file_r = file1_s1d1;
+	const char *const file_w = file2_s1d1;
+	const char *const file_none = file1_s1d2;
+	const struct rule rules[] = {
+		{
+			.path = file_r,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{
+			.path = file_w,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		// Implicitly: No rights for file_none.
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
+			      LANDLOCK_ACCESS_FS_WRITE_FILE;
+	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks read right: truncation should work through truncate and open. */
+	EXPECT_EQ(0, test_truncate(file_r));
+	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
+
+	/* Checks write right: truncation should work through truncate, ftruncate and open. */
+	EXPECT_EQ(0, test_truncate(file_w));
+	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
+
+	/* Checks "no rights" case: truncate works but all open attempts fail. */
+	EXPECT_EQ(0, test_truncate(file_none));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.2

[PATCH v4 3/4] samples/landlock: Extend sample tool to support LANDLOCK_ACCESS_FS_TRUNCATE

[Günther Noack]

Update the sandboxer sample to restrict truncate actions. This is
automatically enabled by default if the running kernel supports
LANDLOCK_ACCESS_FS_TRUNCATE, expect for the paths listed in the
LL_FS_RW environment variable.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 samples/landlock/sandboxer.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 3e404e51ec64..771b6b10d519 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -76,7 +76,8 @@ static int parse_path(char *env_path, const char ***const path_list)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 /* clang-format on */
 
@@ -160,10 +161,8 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	LANDLOCK_ACCESS_FS_REFER)
-
-#define ACCESS_ABI_2 ( \
-	LANDLOCK_ACCESS_FS_REFER)
+	LANDLOCK_ACCESS_FS_REFER | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 /* clang-format on */
 
@@ -226,11 +225,17 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		return 1;
 	}
 	/* Best-effort security. */
-	if (abi < 2) {
-		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
-		access_fs_ro &= ~ACCESS_ABI_2;
-		access_fs_rw &= ~ACCESS_ABI_2;
+	switch (abi) {
+	case 1:
+		/* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+		__attribute__((fallthrough));
+	case 2:
+		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
 	}
+	access_fs_ro &= ruleset_attr.handled_access_fs;
+	access_fs_rw &= ruleset_attr.handled_access_fs;
 
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
-- 
2.37.2

[PATCH v4 4/4] landlock: Document Landlock's file truncation support

[Günther Noack]

Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.

Adapt the backwards compatibility example and discussion to remove the
truncation flag where needed.

Point out potential surprising behaviour related to truncate.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 Documentation/userspace-api/landlock.rst | 48 +++++++++++++++++++-----
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 6648e59fabe7..3ceb97cbe9d1 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
             LANDLOCK_ACCESS_FS_MAKE_FIFO |
             LANDLOCK_ACCESS_FS_MAKE_BLOCK |
             LANDLOCK_ACCESS_FS_MAKE_SYM |
-            LANDLOCK_ACCESS_FS_REFER,
+            LANDLOCK_ACCESS_FS_REFER |
+            LANDLOCK_ACCESS_FS_TRUNCATE,
     };
 
 Because we may not know on which kernel version an application will be
@@ -69,16 +70,26 @@ should try to protect users as much as possible whatever the kernel they are
 using.  To avoid binary enforcement (i.e. either all security features or
 none), we can leverage a dedicated Landlock command to get the current version
 of the Landlock ABI and adapt the handled accesses.  Let's check if we should
-remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
-starting with the second version of the ABI.
+remove the `LANDLOCK_ACCESS_FS_REFER` and `LANDLOCK_ACCESS_FS_TRUNCATE` access
+rights, which are only supported starting with the second and third version of
+the ABI.
 
 .. code-block:: c
 
     int abi;
 
     abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
-    if (abi < 2) {
-        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+    switch (abi) {
+    case -1:
+            perror("The running kernel does not enable to use Landlock");
+            return 1;
+    case 1:
+            /* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
+            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+            __attribute__((fallthrough));
+    case 2:
+            /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
+            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -127,8 +138,8 @@ descriptor.
 
 It may also be required to create rules following the same logic as explained
 for the ruleset creation, by filtering access rights according to the Landlock
-ABI version.  In this example, this is not required because
-`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
+ABI version.  In this example, this is not required because all of the requested
+``allowed_access`` rights are already available in ABI 1.
 
 We now have a ruleset with one rule allowing read access to ``/usr`` while
 denying all other handled accesses for the filesystem.  The next step is to
@@ -251,6 +262,24 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
 process, a sandboxed process should have a subset of the target process rules,
 which means the tracee must be in a sub-domain of the tracer.
 
+Truncating files
+----------------
+
+The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
+`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and sometimes
+overlap in non-intuitive ways.  It is recommended to always specify both of
+these together.
+
+A particularly surprising example is :manpage:`creat(2)`.  The name suggests
+that this system call requires the rights to create and write files.  However,
+it also requires the truncate right if an existing file under the same name is
+already present.
+
+It should also be noted that truncating files does not necessarily require the
+`LANDLOCK_ACCESS_FS_WRITE_FILE` right.  Apart from the obvious
+:manpage:`truncate(2)` system call, this can also be done through
+:manpage:`open(2)` with the flags `O_RDONLY` and `O_TRUNC`.
+
 Compatibility
 =============
 
@@ -386,9 +415,8 @@ File truncation (ABI < 3)
 File truncation could not be denied before the third Landlock ABI, so it is
 always allowed when using a kernel that only supports the first or second ABI.
 
-Starting with the Landlock ABI version 3, it is now possible to securely
-control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access
-right.
+Starting with the Landlock ABI version 3, it is now possible to securely control
+truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access right.
 
 File renaming and linking (ABI 1)
 ---------------------------------
-- 
2.37.2

Re: [PATCH v4 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

On 14/08/2022 21:26, Günther Noack wrote:
> These tests exercise the following truncation operations:
> 
> * truncate() (truncate by path)
> * ftruncate() (truncate by file descriptor)
> * open with the O_TRUNC flag
> * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.
> 
> in the following scenarios:
> 
> * Files with read, write and truncate rights.
> * Files with read and truncate rights.
> * Files with the truncate right.
> * Files without the truncate right.
> 
> In particular, the following scenarios are enforced with the test:
> 
> * The truncate right is required to use ftruncate,
>    even when the thread already has the right to write to the file.
> * open() with O_TRUNC requires the truncate right, if it truncates a file.
>    open() already checks security_path_truncate() in this case,
>    and it required no additional check in the Landlock LSM's file_open hook.
> * creat() requires the truncate right
>    when called with an existing filename.
> * creat() does *not* require the truncate right
>    when it's creating a new file.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 219 +++++++++++++++++++++
>   1 file changed, 219 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..7a2ce6cd1a5a 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
>   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";
>   /* 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";
> @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>    * │           ├── f1
>    * │           └── f2
>    * └── s3d1
> + *     ├── f1
>    *     └── s3d2
>    *         └── s3d3
>    */
> @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
>   	create_file(_metadata, file1_s2d3);
>   	create_file(_metadata, file2_s2d3);
>   
> +	create_file(_metadata, file1_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"));
> @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
>   	EXPECT_EQ(0, remove_path(file1_s2d2));
>   	EXPECT_EQ(0, remove_path(file1_s2d1));
>   
> +	EXPECT_EQ(0, remove_path(file1_s3d1));
>   	EXPECT_EQ(0, remove_path(dir_s3d3));
>   	set_cap(_metadata, CAP_SYS_ADMIN);
>   	umount(dir_s3d2);
> @@ -3023,6 +3027,221 @@ TEST_F_FORK(layout1, proc_pipe)
>   	ASSERT_EQ(0, close(pipe_fds[1]));
>   }
>   
> +/*
> + * Opens the file and invokes ftruncate(2).
> + *
> + * Returns the errno of ftruncate if ftruncate() fails.
> + * Returns EBADFD if open() or close() fail (should not happen).
> + * Returns 0 if ftruncate(), open() and close() were successful.
> + */
> +static int test_ftruncate(struct __test_metadata *const _metadata,

_metadata is no longer needed. Same for test_creat().


> +			  const char *const path, int flags)
> +{
> +	int res, err, fd;
> +
> +	fd = open(path, flags | O_CLOEXEC);
> +	if (fd < 0)
> +		return EBADFD;

Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS 
and add a comment explaining that we are not interested by this open(2) 
error code but only the ftruncate(2) one because we are sure opening 
such path is allowed or because open(2) is already tested before calls 
to test_ftruncate().


> +
> +	res = ftruncate(fd, 10);
> +	err = errno;
> +
> +	if (close(fd) != 0)
> +		return EBADFD;

Why not returning errno? See test_open_rel() comments.


> +
> +	if (res < 0)
> +		return err;
> +	return 0;
> +}
> +
> +/* Invokes truncate(2) and returns the errno or 0. */
> +static int test_truncate(const char *const path)
> +{
> +	if (truncate(path, 10) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/*
> + * Invokes creat(2) and returns its errno or 0.
> + * Closes the opened file descriptor on success.
> + * Returns EBADFD if close() returns an error (should not happen).
> + */
> +static int test_creat(struct __test_metadata *const _metadata,
> +		      const char *const path, mode_t mode)
> +{
> +	int fd = creat(path, mode);
> +
> +	if (fd < 0)
> +		return errno;
> +
> +	if (close(fd) < 0)
> +		return EBADFD;

Why not returning errno?


> +	return 0;
> +}
> +
> +TEST_F_FORK(layout1, truncate)
> +{
> +	const char *const file_rwt = file1_s1d1;
> +	const char *const file_rw = file2_s1d1;
> +	const char *const file_rt = file1_s1d2;
> +	const char *const file_t = file2_s1d2;
> +	const char *const file_none = file1_s1d3;
> +	const char *const dir_t = dir_s2d1;
> +	const char *const file_in_dir_t = file1_s2d1;
> +	const char *const dir_w = dir_s3d1;
> +	const char *const file_in_dir_w = file1_s3d1;
> +	const struct rule rules[] = {
> +		{
> +			.path = file_rwt,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file_rw,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file_rt,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file_t,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		// Implicitly: No access rights for file_none.

Please use /* */ comments everywhere.


> +		{
> +			.path = dir_t,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = dir_w,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> +			      LANDLOCK_ACCESS_FS_WRITE_FILE |
> +			      LANDLOCK_ACCESS_FS_TRUNCATE;
> +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/*
> +	 * Checks read, write and truncate rights: truncation works.
> +	 *
> +	 * Note: Independent of Landlock, ftruncate(2) on read-only
> +	 * file descriptors never works.
> +	 */
> +	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
> +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
> +	EXPECT_EQ(0, test_truncate(file_rwt));
> +	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));

Could you please interleave the test_open() and test_ftruncate() with 
the same open flags, and start with test_open() to make sure assumptions 
are correct (cf. previous comment about test_ftruncate)? Like this 
(everywhere):

EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
EXPECT_EQ(EINVAL, test_ftruncate(file_rwt, O_RDONLY));
EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
EXPECT_EQ(0, test_ftruncate(file_rwt, O_WRONLY));
EXPECT_EQ(0, test_truncate(file_rwt));


> +	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> +
> +	/* Checks read and write rights: no truncate variant works. */
> +	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
> +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_truncate(file_rw));
> +	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
> +
> +	/*
> +	 * Checks read and truncate rights: truncation works.
> +	 *
> +	 * Note: Files opened in O_RDONLY can get truncated as part of
> +	 * the same operation.
> +	 */
> +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
> +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));

Why not a test_ftruncate() check here?


> +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));

There is a missing "| O_TRUNC"


> +	EXPECT_EQ(0, test_truncate(file_rt));
> +
> +	/* Checks truncate right: truncate works, but can't open file. */
> +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_truncate(file_t));
> +
> +	/* Checks "no rights" case: No form of truncation works. */
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_truncate(file_none));
> +
> +	/* Checks truncate right on directory: truncate works on contained files */
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_truncate(file_in_dir_t));
> +
> +	/*
> +	 * Checks creat in dir_w: This requires the truncate right
> +	 * when overwriting an existing file, but does not require it
> +	 * when the file is new.
> +	 */
> +	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
> +
> +	ASSERT_EQ(0, unlink(file_in_dir_w));
> +	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
> +}
> +
> +/*
> + * Exercises file truncation when it's not restricted,
> + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
> + */
> +TEST_F_FORK(layout1, truncate_unhandled)

Can you move layout1.truncate_unhandled tests before layout1.truncate?


> +{
> +	const char *const file_r = file1_s1d1;
> +	const char *const file_w = file2_s1d1;
> +	const char *const file_none = file1_s1d2;
> +	const struct rule rules[] = {
> +		{
> +			.path = file_r,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{
> +			.path = file_w,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		// Implicitly: No rights for file_none.
> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> +			      LANDLOCK_ACCESS_FS_WRITE_FILE;
> +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks read right: truncation should work through truncate and open. */
> +	EXPECT_EQ(0, test_truncate(file_r));

We should be consistent to also check with test_ftruncate() (and order 
the EXPECT checks appropriately).


> +	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
> +
> +	/* Checks write right: truncation should work through truncate, ftruncate and open. */

Please align to 80 columns.


> +	EXPECT_EQ(0, test_truncate(file_w));
> +	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
> +
> +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> +	EXPECT_EQ(0, test_truncate(file_none));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));

Can you test with test_creat() here?


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

Re: [PATCH v4 4/4] landlock: Document Landlock's file truncation support

[Mickaël Salaün]

On 14/08/2022 21:26, Günther Noack wrote:
> Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.
> 
> Adapt the backwards compatibility example and discussion to remove the
> truncation flag where needed.
> 
> Point out potential surprising behaviour related to truncate.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   Documentation/userspace-api/landlock.rst | 48 +++++++++++++++++++-----
>   1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 6648e59fabe7..3ceb97cbe9d1 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
>               LANDLOCK_ACCESS_FS_MAKE_FIFO |
>               LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>               LANDLOCK_ACCESS_FS_MAKE_SYM |
> -            LANDLOCK_ACCESS_FS_REFER,
> +            LANDLOCK_ACCESS_FS_REFER |
> +            LANDLOCK_ACCESS_FS_TRUNCATE,
>       };
>   
>   Because we may not know on which kernel version an application will be
> @@ -69,16 +70,26 @@ should try to protect users as much as possible whatever the kernel they are
>   using.  To avoid binary enforcement (i.e. either all security features or
>   none), we can leverage a dedicated Landlock command to get the current version
>   of the Landlock ABI and adapt the handled accesses.  Let's check if we should
> -remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
> -starting with the second version of the ABI.
> +remove the `LANDLOCK_ACCESS_FS_REFER` and `LANDLOCK_ACCESS_FS_TRUNCATE` access

s/and/or/

> +rights, which are only supported starting with the second and third version of
> +the ABI.
>   
>   .. code-block:: c
>   
>       int abi;
>   
>       abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
> -    if (abi < 2) {
> -        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> +    switch (abi) {
> +    case -1:
> +            perror("The running kernel does not enable to use Landlock");
> +            return 1;
> +    case 1:
> +            /* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
> +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> +            __attribute__((fallthrough));
> +    case 2:
> +            /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
> +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
>       }
>   
>   This enables to create an inclusive ruleset that will contain our rules.
> @@ -127,8 +138,8 @@ descriptor.
>   
>   It may also be required to create rules following the same logic as explained
>   for the ruleset creation, by filtering access rights according to the Landlock
> -ABI version.  In this example, this is not required because
> -`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
> +ABI version.  In this example, this is not required because all of the requested
> +``allowed_access`` rights are already available in ABI 1.
>   
>   We now have a ruleset with one rule allowing read access to ``/usr`` while
>   denying all other handled accesses for the filesystem.  The next step is to
> @@ -251,6 +262,24 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
>   process, a sandboxed process should have a subset of the target process rules,
>   which means the tracee must be in a sub-domain of the tracer.
>   
> +Truncating files
> +----------------
> +
> +The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
> +`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and sometimes
> +overlap in non-intuitive ways.  It is recommended to always specify both of
> +these together.
> +
> +A particularly surprising example is :manpage:`creat(2)`.  The name suggests
> +that this system call requires the rights to create and write files.  However,
> +it also requires the truncate right if an existing file under the same name is
> +already present.
> +
> +It should also be noted that truncating files does not necessarily require the

I think "necessarily" is superfluous here.


> +`LANDLOCK_ACCESS_FS_WRITE_FILE` right.  Apart from the obvious
> +:manpage:`truncate(2)` system call, this can also be done through
> +:manpage:`open(2)` with the flags `O_RDONLY` and `O_TRUNC`.

`O_RDONLY | O_TRUNC`.


> +
>   Compatibility
>   =============
>   
> @@ -386,9 +415,8 @@ File truncation (ABI < 3)
>   File truncation could not be denied before the third Landlock ABI, so it is
>   always allowed when using a kernel that only supports the first or second ABI.
>   
> -Starting with the Landlock ABI version 3, it is now possible to securely
> -control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access
> -right.
> +Starting with the Landlock ABI version 3, it is now possible to securely control
> +truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access right.

This is an inconsistent hunk, patching the first patch.

Please also move this "File truncation" section below the "File renaming 
and linking".


>   
>   File renaming and linking (ABI 1)
>   ---------------------------------

Re: [PATCH v4 1/4] landlock: Support file truncation

[Mickaël Salaün]

On 14/08/2022 21:26, Günther Noack wrote:
> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> 
> This flag hooks into the path_truncate LSM hook and covers file
> truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
> well as creat().
> 
> This change also increments the Landlock ABI version, updates
> corresponding selftests, and includes minor documentation changes to
> document the flag.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   Documentation/userspace-api/landlock.rst     | 10 ++++++++++
>   include/uapi/linux/landlock.h                | 17 ++++++++++++-----
>   security/landlock/fs.c                       |  9 ++++++++-
>   security/landlock/limits.h                   |  2 +-
>   security/landlock/syscalls.c                 |  2 +-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   tools/testing/selftests/landlock/fs_test.c   |  7 ++++---
>   7 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index b8ea59493964..6648e59fabe7 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -380,6 +380,16 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
>   Previous limitations
>   ====================
>   
> +File truncation (ABI < 3)
> +-------------------------
> +
> +File truncation could not be denied before the third Landlock ABI, so it is
> +always allowed when using a kernel that only supports the first or second ABI.
> +
> +Starting with the Landlock ABI version 3, it is now possible to securely
> +control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access
> +right.
> +

This should be in the forth patch, below the file renaming and linking 
section.


>   File renaming and linking (ABI 1)
>   ---------------------------------
>   
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..a2fef267bf34 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
>    * A file can only receive these access rights:
>    *
>    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> - * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
> + * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
> + *   you might additionally need the `LANDLOCK_ACCESS_FS_TRUNCATE` right in
> + *   order to overwrite files with :manpage:`open(2)` using `O_TRUNC` or
> + *   :manpage:`creat(2)`.
>    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation APIs
> + *   like :manpage:`truncate(2)`, :manpage:`ftruncate(2)`, or

s/through file truncation APIs like/with/


> + *   :manpage:`open(2)` with `O_TRUNC` or :manpage:`creat(2)`. This access right

:manpage:`creat(2)`, or :manpage:`open(2)` using `O_TRUNC`.

With only one "or".


> + *   is available since the third version of the Landlock ABI.
>    *
>    * A directory can receive access rights related to files or directories.  The
>    * following access right is applied to the directory itself, and the
> @@ -139,10 +146,9 @@ struct landlock_path_beneath_attr {
>    *
>    *   It is currently not possible to restrict some file-related actions
>    *   accessible through these syscall families: :manpage:`chdir(2)`,
> - *   :manpage:`truncate(2)`, :manpage:`stat(2)`, :manpage:`flock(2)`,
> - *   :manpage:`chmod(2)`, :manpage:`chown(2)`, :manpage:`setxattr(2)`,
> - *   :manpage:`utime(2)`, :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
> - *   :manpage:`access(2)`.
> + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
> + *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> + *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
>    *   Future Landlock evolutions will enable to restrict them.
>    */
>   /* clang-format off */
> @@ -160,6 +166,7 @@ struct landlock_path_beneath_attr {
>   #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
> +#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>   /* clang-format on */
>   
>   #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ec5a6247cd3e..c57f581a9cd5 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>   #define ACCESS_FILE ( \
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> -	LANDLOCK_ACCESS_FS_READ_FILE)
> +	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   /* clang-format on */
>   
>   /*
> @@ -1140,6 +1141,11 @@ static int hook_path_rmdir(const struct path *const dir,
>   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
>   }
>   
> +static int hook_path_truncate(const struct path *const path)
> +{
> +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> +}
> +
>   /* File hooks */
>   
>   static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1192,6 +1198,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
>   	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>   	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
> +	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
>   
>   	LSM_HOOK_INIT(file_open, hook_file_open),
>   };
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index b54184ab9439..82288f0e9e5e 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>   #define LANDLOCK_MAX_NUM_LAYERS		16
>   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>   
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_REFER
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>   
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 735a0865ea11..f4d6fc7ed17f 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>   	.write = fop_dummy_write,
>   };
>   
> -#define LANDLOCK_ABI_VERSION 2
> +#define LANDLOCK_ABI_VERSION 3
>   
>   /**
>    * sys_landlock_create_ruleset - Create a new ruleset
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index da9290817866..72cdae277b02 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>   	const struct landlock_ruleset_attr ruleset_attr = {
>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>   	};
> -	ASSERT_EQ(2, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
>   					     LANDLOCK_CREATE_RULESET_VERSION));
>   
>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 21a2ce8fa739..cb77eaa01c91 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -399,9 +399,10 @@ TEST_F_FORK(layout1, inval)
>   #define ACCESS_FILE ( \
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> -	LANDLOCK_ACCESS_FS_READ_FILE)
> +	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
>   
>   #define ACCESS_ALL ( \
>   	ACCESS_FILE | \
> @@ -415,7 +416,7 @@ TEST_F_FORK(layout1, inval)
>   	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
>   	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
>   	LANDLOCK_ACCESS_FS_MAKE_SYM | \
> -	ACCESS_LAST)
> +	LANDLOCK_ACCESS_FS_REFER)
>   
>   /* clang-format on */
>   

Re: [PATCH v4 1/4] landlock: Support file truncation

[Günther Noack]

On Tue, Aug 16, 2022 at 09:20:06PM +0200, Mickaël Salaün wrote:
> On 14/08/2022 21:26, Günther Noack wrote:
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index b8ea59493964..6648e59fabe7 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> > @@ -380,6 +380,16 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
> >   Previous limitations
> >   ====================
> > +File truncation (ABI < 3)
> > +-------------------------
> > +
> > +File truncation could not be denied before the third Landlock ABI, so it is
> > +always allowed when using a kernel that only supports the first or second ABI.
> > +
> > +Starting with the Landlock ABI version 3, it is now possible to securely
> > +control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access
> > +right.
> > +
>
> This should be in the forth patch, below the file renaming and linking
> section.

Good point, moved.

> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..a2fef267bf34 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
> >    * A file can only receive these access rights:
> >    *
> >    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> > - * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
> > + * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
> > + *   you might additionally need the `LANDLOCK_ACCESS_FS_TRUNCATE` right in
> > + *   order to overwrite files with :manpage:`open(2)` using `O_TRUNC` or
> > + *   :manpage:`creat(2)`.
> >    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> > + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation APIs
> > + *   like :manpage:`truncate(2)`, :manpage:`ftruncate(2)`, or
>
> s/through file truncation APIs like/with/

Done.

>
>
> > + *   :manpage:`open(2)` with `O_TRUNC` or :manpage:`creat(2)`. This access right
>
> :manpage:`creat(2)`, or :manpage:`open(2)` using `O_TRUNC`.
>
> With only one "or".

Done.

Thanks for the attention to detail in the review!

—Günther

--

Re: [PATCH v4 2/4] selftests/landlock: Selftests for file truncation support

[Günther Noack]

On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote:
> On 14/08/2022 21:26, Günther Noack wrote:
> > +/*
> > + * Opens the file and invokes ftruncate(2).
> > + *
> > + * Returns the errno of ftruncate if ftruncate() fails.
> > + * Returns EBADFD if open() or close() fail (should not happen).
> > + * Returns 0 if ftruncate(), open() and close() were successful.
> > + */
> > +static int test_ftruncate(struct __test_metadata *const _metadata,
>
> _metadata is no longer needed. Same for test_creat().

Thanks, well spotted!

>
>
> > +			  const char *const path, int flags)
> > +{
> > +	int res, err, fd;
> > +
> > +	fd = open(path, flags | O_CLOEXEC);
> > +	if (fd < 0)
> > +		return EBADFD;
>
> Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and
> add a comment explaining that we are not interested by this open(2) error
> code but only the ftruncate(2) one because we are sure opening such path is
> allowed or because open(2) is already tested before calls to
> test_ftruncate().

Changed to ENOSYS and added a comment in the code and as function documentation.

The explanation follows the reasoning that callers must guarantee that
open() and close() will work, in order to test ftruncate() correctly.
If open() or close() fail, we return ENOSYS.

Technically EBADFD does not get returned by open(2) according to the
man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy
to mix up).

> > +
> > +	res = ftruncate(fd, 10);
> > +	err = errno;
> > +
> > +	if (close(fd) != 0)
> > +		return EBADFD;
>
> Why not returning errno? See test_open_rel() comments.

OK, changed to return errno for consistency, with the same comment.

>
>
> > +
> > +	if (res < 0)
> > +		return err;
> > +	return 0;
> > +}
> > +
> > +/* Invokes truncate(2) and returns the errno or 0. */
> > +static int test_truncate(const char *const path)
> > +{
> > +	if (truncate(path, 10) < 0)
> > +		return errno;
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Invokes creat(2) and returns its errno or 0.
> > + * Closes the opened file descriptor on success.
> > + * Returns EBADFD if close() returns an error (should not happen).
> > + */
> > +static int test_creat(struct __test_metadata *const _metadata,
> > +		      const char *const path, mode_t mode)
> > +{
> > +	int fd = creat(path, mode);
> > +
> > +	if (fd < 0)
> > +		return errno;
> > +
> > +	if (close(fd) < 0)
> > +		return EBADFD;
>
> Why not returning errno?

Done. (Same)

> > +		// Implicitly: No access rights for file_none.
>
> Please use /* */ comments everywhere.

Done. (in both places)

> > +	/*
> > +	 * Checks read, write and truncate rights: truncation works.
> > +	 *
> > +	 * Note: Independent of Landlock, ftruncate(2) on read-only
> > +	 * file descriptors never works.
> > +	 */
> > +	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
> > +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
> > +	EXPECT_EQ(0, test_truncate(file_rwt));
> > +	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
>
> Could you please interleave the test_open() and test_ftruncate() with the
> same open flags, and start with test_open() to make sure assumptions are
> correct (cf. previous comment about test_ftruncate)? Like this (everywhere):
>
> EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> EXPECT_EQ(EINVAL, test_ftruncate(file_rwt, O_RDONLY));
> EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
> EXPECT_EQ(0, test_ftruncate(file_rwt, O_WRONLY));
> EXPECT_EQ(0, test_truncate(file_rwt));

OK, I ordered it like that.

When calling

  EXPECT_EQ(foo, test_ftruncate(...));

and "foo" is not ENOSYS (which it should never be!), then that alone
is enough to guarantee that the open() in test_ftruncate worked, so in
a sense this is already tested implicitly.

The only thing to make sure is to never *expect* ENOSYS from
test_ftruncate(), but I documented in test_ftruncate() now that it is
better to use test_open() for testing the result of open().

>
>
> > +	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> > +
> > +	/* Checks read and write rights: no truncate variant works. */
> > +	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
> > +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_truncate(file_rw));
> > +	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
> > +
> > +	/*
> > +	 * Checks read and truncate rights: truncation works.
> > +	 *
> > +	 * Note: Files opened in O_RDONLY can get truncated as part of
> > +	 * the same operation.
> > +	 */
> > +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
> > +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
>
> Why not a test_ftruncate() check here?

The twist here is, althrough truncate() works, ftruncate() cannot
truncate the file in this case:

(a) when trying to open the file for writing, Landlock policy keeps
    you from doing that (tested with test_open).

(b) when opening the file read-only, that works, but read-only fds can
    never be ftruncated (explained in the man page).

It's slightly surprising when just glancing over the test, so I added
the check for extra clarity:

EXPECT_EQ(EINVAL, test_ftruncate(file_rt, O_RDONLY));

The check for test_open with O_WRONLY was already there.

> > +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
>
> There is a missing "| O_TRUNC"

Well spotted! Fixed.

> > +	EXPECT_EQ(0, test_truncate(file_rt));
> > +
> > +	/* Checks truncate right: truncate works, but can't open file. */
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_truncate(file_t));
> > +
> > +	/* Checks "no rights" case: No form of truncation works. */
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_truncate(file_none));
> > +
> > +	/* Checks truncate right on directory: truncate works on contained files */
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_truncate(file_in_dir_t));
> > +
> > +	/*
> > +	 * Checks creat in dir_w: This requires the truncate right
> > +	 * when overwriting an existing file, but does not require it
> > +	 * when the file is new.
> > +	 */
> > +	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
> > +
> > +	ASSERT_EQ(0, unlink(file_in_dir_w));
> > +	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
> > +}
> > +
> > +/*
> > + * Exercises file truncation when it's not restricted,
> > + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
> > + */
> > +TEST_F_FORK(layout1, truncate_unhandled)
>
> Can you move layout1.truncate_unhandled tests before layout1.truncate?

Done.

>
>
> > +{
> > +	const char *const file_r = file1_s1d1;
> > +	const char *const file_w = file2_s1d1;
> > +	const char *const file_none = file1_s1d2;
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = file_r,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> > +		},
> > +		{
> > +			.path = file_w,
> > +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +		},
> > +		// Implicitly: No rights for file_none.
> > +		{},
> > +	};
> > +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> > +			      LANDLOCK_ACCESS_FS_WRITE_FILE;
> > +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> > +
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Checks read right: truncation should work through truncate and open. */
> > +	EXPECT_EQ(0, test_truncate(file_r));
>
> We should be consistent to also check with test_ftruncate() (and order the
> EXPECT checks appropriately).

Added.

>
>
> > +	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
> > +
> > +	/* Checks write right: truncation should work through truncate, ftruncate and open. */
>
> Please align to 80 columns.

Done.

>
>
> > +	EXPECT_EQ(0, test_truncate(file_w));
> > +	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
> > +
> > +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> > +	EXPECT_EQ(0, test_truncate(file_none));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
>
> Can you test with test_creat() here?

Added.

Thanks for the careful review!
—Günther

--

Re: [PATCH v4 4/4] landlock: Document Landlock's file truncation support

[Günther Noack]

On Tue, Aug 16, 2022 at 09:18:33PM +0200, Mickaël Salaün wrote:
> On 14/08/2022 21:26, Günther Noack wrote:
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index 6648e59fabe7..3ceb97cbe9d1 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> >   Because we may not know on which kernel version an application will be
> > @@ -69,16 +70,26 @@ should try to protect users as much as possible whatever the kernel they are
> >   using.  To avoid binary enforcement (i.e. either all security features or
> >   none), we can leverage a dedicated Landlock command to get the current version
> >   of the Landlock ABI and adapt the handled accesses.  Let's check if we should
> > -remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
> > -starting with the second version of the ABI.
> > +remove the `LANDLOCK_ACCESS_FS_REFER` and `LANDLOCK_ACCESS_FS_TRUNCATE` access
>
> s/and/or/

Done.

> > +Truncating files
> > +----------------
> > +
> > +The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
> > +`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and sometimes
> > +overlap in non-intuitive ways.  It is recommended to always specify both of
> > +these together.
> > +
> > +A particularly surprising example is :manpage:`creat(2)`.  The name suggests
> > +that this system call requires the rights to create and write files.  However,
> > +it also requires the truncate right if an existing file under the same name is
> > +already present.
> > +
> > +It should also be noted that truncating files does not necessarily require the
>
> I think "necessarily" is superfluous here.

Done.  I dropped the "obvious" too.

>
>
> > +`LANDLOCK_ACCESS_FS_WRITE_FILE` right.  Apart from the obvious
> > +:manpage:`truncate(2)` system call, this can also be done through
> > +:manpage:`open(2)` with the flags `O_RDONLY` and `O_TRUNC`.
>
> `O_RDONLY | O_TRUNC`.

Done.

> >   Compatibility
> >   =============
> > @@ -386,9 +415,8 @@ File truncation (ABI < 3)
> >   File truncation could not be denied before the third Landlock ABI, so it is
> >   always allowed when using a kernel that only supports the first or second ABI.
> > -Starting with the Landlock ABI version 3, it is now possible to securely
> > -control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access
> > -right.
> > +Starting with the Landlock ABI version 3, it is now possible to securely control
> > +truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access right.
>
> This is an inconsistent hunk, patching the first patch.
>
> Please also move this "File truncation" section below the "File renaming and
> linking".

Thanks, fixed the ordering of commits and moved the truncation section
below "File Renaming and Linking".

—Günther

--

Re: [PATCH v4 2/4] selftests/landlock: Selftests for file truncation support

[Günther Noack]

On Wed, Aug 17, 2022 at 08:00:17PM +0200, Günther Noack wrote:
> On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote:
> > On 14/08/2022 21:26, Günther Noack wrote:
> > > +/*
> > > + * Opens the file and invokes ftruncate(2).
> > > + *
> > > + * Returns the errno of ftruncate if ftruncate() fails.
> > > + * Returns EBADFD if open() or close() fail (should not happen).
> > > + * Returns 0 if ftruncate(), open() and close() were successful.
> > > + */
> > > +static int test_ftruncate(struct __test_metadata *const _metadata,
> >
> > _metadata is no longer needed. Same for test_creat().
>
> Thanks, well spotted!
>
> >
> >
> > > +			  const char *const path, int flags)
> > > +{
> > > +	int res, err, fd;
> > > +
> > > +	fd = open(path, flags | O_CLOEXEC);
> > > +	if (fd < 0)
> > > +		return EBADFD;
> >
> > Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and
> > add a comment explaining that we are not interested by this open(2) error
> > code but only the ftruncate(2) one because we are sure opening such path is
> > allowed or because open(2) is already tested before calls to
> > test_ftruncate().
>
> Changed to ENOSYS and added a comment in the code and as function documentation.
>
> The explanation follows the reasoning that callers must guarantee that
> open() and close() will work, in order to test ftruncate() correctly.
> If open() or close() fail, we return ENOSYS.
>
> Technically EBADFD does not get returned by open(2) according to the
> man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy
> to mix up).

The more I think about it, the more I feel that test_ftruncate() in its current
form was a mistake:

  * In reality, we just care about the ftruncate() result, not about open().
  * The tests became slightly confusing and asymmetric because some
    places could call test_ftruncate() while others would call test_open()
  * Trying open(..., O_RDONLY) + ftruncate() is also confusing in tests,
    because that never works anyway (ftruncate() only works on writable fds)

So: I'm contemplating to use a different approach instead:

  * Open a writable FD for each file *before enforcing Landlock*.
  * *Then* enforce Landlock (now some of these files can't be opened any more)
  * Then try ftruncate() with the previously opened file descriptor.

As a result,

  * we have some new repetitive but simple code for opening file descriptors
  * we remove the long version of test_ftruncate() with all its error case
    complication and replace it with a trivial one that takes an already-opened
    file descriptor.

This way, each block in the test now checks the following things:

  * check truncate(file)
  * check ftruncate(file_fd) <--- passing the FD!
  * check open(file, O_RDONLY|O_TRUNC)
  * check open(file, O_WRONLY|O_TRUNC)

It's now easy to see in the tests that the result from truncate() and
ftruncate() is always the same. The complication of worrying whether open()
works before ftruncate() is also gone (so no more special open() checks needed
before checking ftruncate()). I removed the testing of ftruncate() on read-only
file descriptors, because that is forbidden in the ftruncate() manpage anyway
and always returns EINVAL independent of Landlock.

I feel that this helps clarify the tests, even if it undoes some of your
comments about expecting open() to work before ftruncate().

Does that approach look reasonable to you?

I might just send you a patch version with that variant I think - this might be
clearer in code than in my textual description here. :)

—Günther

>
> > > +
> > > +	res = ftruncate(fd, 10);
> > > +	err = errno;
> > > +
> > > +	if (close(fd) != 0)
> > > +		return EBADFD;
> >
> > Why not returning errno? See test_open_rel() comments.
>
> OK, changed to return errno for consistency, with the same comment.
>
> >
> >
> > > +
> > > +	if (res < 0)
> > > +		return err;
> > > +	return 0;
> > > +}
> > > +
> > > +/* Invokes truncate(2) and returns the errno or 0. */
> > > +static int test_truncate(const char *const path)
> > > +{
> > > +	if (truncate(path, 10) < 0)
> > > +		return errno;
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Invokes creat(2) and returns its errno or 0.
> > > + * Closes the opened file descriptor on success.
> > > + * Returns EBADFD if close() returns an error (should not happen).
> > > + */
> > > +static int test_creat(struct __test_metadata *const _metadata,
> > > +		      const char *const path, mode_t mode)
> > > +{
> > > +	int fd = creat(path, mode);
> > > +
> > > +	if (fd < 0)
> > > +		return errno;
> > > +
> > > +	if (close(fd) < 0)
> > > +		return EBADFD;
> >
> > Why not returning errno?
>
> Done. (Same)
>
> > > +		// Implicitly: No access rights for file_none.
> >
> > Please use /* */ comments everywhere.
>
> Done. (in both places)
>
> > > +	/*
> > > +	 * Checks read, write and truncate rights: truncation works.
> > > +	 *
> > > +	 * Note: Independent of Landlock, ftruncate(2) on read-only
> > > +	 * file descriptors never works.
> > > +	 */
> > > +	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
> > > +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
> > > +	EXPECT_EQ(0, test_truncate(file_rwt));
> > > +	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
> >
> > Could you please interleave the test_open() and test_ftruncate() with the
> > same open flags, and start with test_open() to make sure assumptions are
> > correct (cf. previous comment about test_ftruncate)? Like this (everywhere):
> >
> > EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> > EXPECT_EQ(EINVAL, test_ftruncate(file_rwt, O_RDONLY));
> > EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
> > EXPECT_EQ(0, test_ftruncate(file_rwt, O_WRONLY));
> > EXPECT_EQ(0, test_truncate(file_rwt));
>
> OK, I ordered it like that.
>
> When calling
>
>   EXPECT_EQ(foo, test_ftruncate(...));
>
> and "foo" is not ENOSYS (which it should never be!), then that alone
> is enough to guarantee that the open() in test_ftruncate worked, so in
> a sense this is already tested implicitly.
>
> The only thing to make sure is to never *expect* ENOSYS from
> test_ftruncate(), but I documented in test_ftruncate() now that it is
> better to use test_open() for testing the result of open().
>
> >
> >
> > > +	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> > > +
> > > +	/* Checks read and write rights: no truncate variant works. */
> > > +	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
> > > +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
> > > +	EXPECT_EQ(EACCES, test_truncate(file_rw));
> > > +	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
> > > +
> > > +	/*
> > > +	 * Checks read and truncate rights: truncation works.
> > > +	 *
> > > +	 * Note: Files opened in O_RDONLY can get truncated as part of
> > > +	 * the same operation.
> > > +	 */
> > > +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
> > > +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
> >
> > Why not a test_ftruncate() check here?
>
> The twist here is, althrough truncate() works, ftruncate() cannot
> truncate the file in this case:
>
> (a) when trying to open the file for writing, Landlock policy keeps
>     you from doing that (tested with test_open).
>
> (b) when opening the file read-only, that works, but read-only fds can
>     never be ftruncated (explained in the man page).
>
> It's slightly surprising when just glancing over the test, so I added
> the check for extra clarity:
>
> EXPECT_EQ(EINVAL, test_ftruncate(file_rt, O_RDONLY));
>
> The check for test_open with O_WRONLY was already there.
>
> > > +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> >
> > There is a missing "| O_TRUNC"
>
> Well spotted! Fixed.
>
> > > +	EXPECT_EQ(0, test_truncate(file_rt));
> > > +
> > > +	/* Checks truncate right: truncate works, but can't open file. */
> > > +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
> > > +	EXPECT_EQ(0, test_truncate(file_t));
> > > +
> > > +	/* Checks "no rights" case: No form of truncation works. */
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_truncate(file_none));
> > > +
> > > +	/* Checks truncate right on directory: truncate works on contained files */
> > > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
> > > +	EXPECT_EQ(0, test_truncate(file_in_dir_t));
> > > +
> > > +	/*
> > > +	 * Checks creat in dir_w: This requires the truncate right
> > > +	 * when overwriting an existing file, but does not require it
> > > +	 * when the file is new.
> > > +	 */
> > > +	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
> > > +
> > > +	ASSERT_EQ(0, unlink(file_in_dir_w));
> > > +	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
> > > +}
> > > +
> > > +/*
> > > + * Exercises file truncation when it's not restricted,
> > > + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
> > > + */
> > > +TEST_F_FORK(layout1, truncate_unhandled)
> >
> > Can you move layout1.truncate_unhandled tests before layout1.truncate?
>
> Done.
>
> >
> >
> > > +{
> > > +	const char *const file_r = file1_s1d1;
> > > +	const char *const file_w = file2_s1d1;
> > > +	const char *const file_none = file1_s1d2;
> > > +	const struct rule rules[] = {
> > > +		{
> > > +			.path = file_r,
> > > +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> > > +		},
> > > +		{
> > > +			.path = file_w,
> > > +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> > > +		},
> > > +		// Implicitly: No rights for file_none.
> > > +		{},
> > > +	};
> > > +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> > > +			      LANDLOCK_ACCESS_FS_WRITE_FILE;
> > > +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> > > +
> > > +	ASSERT_LE(0, ruleset_fd);
> > > +	enforce_ruleset(_metadata, ruleset_fd);
> > > +	ASSERT_EQ(0, close(ruleset_fd));
> > > +
> > > +	/* Checks read right: truncation should work through truncate and open. */
> > > +	EXPECT_EQ(0, test_truncate(file_r));
> >
> > We should be consistent to also check with test_ftruncate() (and order the
> > EXPECT checks appropriately).
>
> Added.
>
> >
> >
> > > +	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
> > > +
> > > +	/* Checks write right: truncation should work through truncate, ftruncate and open. */
> >
> > Please align to 80 columns.
>
> Done.
>
> >
> >
> > > +	EXPECT_EQ(0, test_truncate(file_w));
> > > +	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
> > > +	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
> > > +	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
> > > +
> > > +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> > > +	EXPECT_EQ(0, test_truncate(file_none));
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> > > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> >
> > Can you test with test_creat() here?
>
> Added.
>
> Thanks for the careful review!
> —Günther
>
> --

--

Re: [PATCH v4 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

On 17/08/2022 21:35, Günther Noack wrote:
> On Wed, Aug 17, 2022 at 08:00:17PM +0200, Günther Noack wrote:
>> On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote:
>>> On 14/08/2022 21:26, Günther Noack wrote:
>>>> +/*
>>>> + * Opens the file and invokes ftruncate(2).
>>>> + *
>>>> + * Returns the errno of ftruncate if ftruncate() fails.
>>>> + * Returns EBADFD if open() or close() fail (should not happen).
>>>> + * Returns 0 if ftruncate(), open() and close() were successful.
>>>> + */
>>>> +static int test_ftruncate(struct __test_metadata *const _metadata,
>>>
>>> _metadata is no longer needed. Same for test_creat().
>>
>> Thanks, well spotted!
>>
>>>
>>>
>>>> +			  const char *const path, int flags)
>>>> +{
>>>> +	int res, err, fd;
>>>> +
>>>> +	fd = open(path, flags | O_CLOEXEC);
>>>> +	if (fd < 0)
>>>> +		return EBADFD;
>>>
>>> Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and
>>> add a comment explaining that we are not interested by this open(2) error
>>> code but only the ftruncate(2) one because we are sure opening such path is
>>> allowed or because open(2) is already tested before calls to
>>> test_ftruncate().
>>
>> Changed to ENOSYS and added a comment in the code and as function documentation.
>>
>> The explanation follows the reasoning that callers must guarantee that
>> open() and close() will work, in order to test ftruncate() correctly.
>> If open() or close() fail, we return ENOSYS.
>>
>> Technically EBADFD does not get returned by open(2) according to the
>> man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy
>> to mix up).
> 
> The more I think about it, the more I feel that test_ftruncate() in its current
> form was a mistake:
> 
>    * In reality, we just care about the ftruncate() result, not about open().
>    * The tests became slightly confusing and asymmetric because some
>      places could call test_ftruncate() while others would call test_open()
>    * Trying open(..., O_RDONLY) + ftruncate() is also confusing in tests,
>      because that never works anyway (ftruncate() only works on writable fds)
> 
> So: I'm contemplating to use a different approach instead:
> 
>    * Open a writable FD for each file *before enforcing Landlock*.
>    * *Then* enforce Landlock (now some of these files can't be opened any more)
>    * Then try ftruncate() with the previously opened file descriptor.
> 
> As a result,
> 
>    * we have some new repetitive but simple code for opening file descriptors
>    * we remove the long version of test_ftruncate() with all its error case
>      complication and replace it with a trivial one that takes an already-opened
>      file descriptor.
> 
> This way, each block in the test now checks the following things:
> 
>    * check truncate(file)
>    * check ftruncate(file_fd) <--- passing the FD!
>    * check open(file, O_RDONLY|O_TRUNC)
>    * check open(file, O_WRONLY|O_TRUNC)
> 
> It's now easy to see in the tests that the result from truncate() and
> ftruncate() is always the same. The complication of worrying whether open()
> works before ftruncate() is also gone (so no more special open() checks needed
> before checking ftruncate()). I removed the testing of ftruncate() on read-only
> file descriptors, because that is forbidden in the ftruncate() manpage anyway
> and always returns EINVAL independent of Landlock.
> 
> I feel that this helps clarify the tests, even if it undoes some of your
> comments about expecting open() to work before ftruncate().
> 
> Does that approach look reasonable to you?
> 
> I might just send you a patch version with that variant I think - this might be
> clearer in code than in my textual description here. :)

The FD from the pre-landlocked state is the right approach, thanks!