[PATCH v2 0/4] landlock: truncate support

[PATCH v2 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 uses of creat() that overwrite existing regular files.

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-rc6.

Best regards,
Günther

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

History
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/

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     |  24 +-
 include/uapi/linux/landlock.h                |  13 +-
 samples/landlock/sandboxer.c                 |  15 +-
 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   | 245 ++++++++++++++++++-
 8 files changed, 294 insertions(+), 18 deletions(-)

--
2.37.0

[PATCH v2 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>
Link: https://lore.kernel.org/all/20220707200612.132705-2-gnoack3000@gmail.com/
---
 Documentation/userspace-api/landlock.rst     |  5 +++++
 include/uapi/linux/landlock.h                | 13 +++++++++----
 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, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index b8ea59493964..b86fd94ae797 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -380,6 +380,11 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
 Previous limitations
 ====================
 
+File truncation (ABI 1-2)
+-------------------------
+
+File truncation was not restrictable in ABI 1-2, so it was always permitted.
+
 File renaming and linking (ABI 1)
 ---------------------------------
 
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 23df4e0e8ace..9ca7f9d0d862 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -96,7 +96,12 @@ struct landlock_path_beneath_attr {
  *
  * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
  * - %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 open(2) using O_TRUNC or 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 truncate(2), ftruncate(2), open(2) with O_TRUNC or 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 +144,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 +164,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.0

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

[Günther Noack]

These tests exercise the following truncation operations:

* truncate() and trunate64() (truncate by path)
* ftruncate() and ftruncate64() (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>
Link: https://lore.kernel.org/all/20220707200612.132705-3-gnoack3000@gmail.com/
---
 tools/testing/selftests/landlock/fs_test.c | 238 +++++++++++++++++++++
 1 file changed, 238 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index cb77eaa01c91..1e5660118bce 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3023,6 +3023,244 @@ TEST_F_FORK(layout1, proc_pipe)
 	ASSERT_EQ(0, close(pipe_fds[1]));
 }
 
+TEST_F_FORK(layout1, truncate)
+{
+	const char *file_rwt = file1_s1d1;
+	const char *file_rw = file2_s1d1;
+	const char *file_rt = file1_s1d2;
+	const char *file_t = file2_s1d2;
+	const char *file_none = file1_s1d3;
+	const char *dir_t = dir_s2d1;
+	const char *file_in_dir_t = file1_s2d1;
+	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,
+		},
+		{},
+	};
+	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);
+	struct stat statbuf;
+	int reg_fd;
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks read, write and truncate rights: truncate and ftruncate work. */
+	reg_fd = open(file_rwt, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	EXPECT_EQ(0, ftruncate(reg_fd, 10));
+	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
+	ASSERT_EQ(0, close(reg_fd));
+
+	EXPECT_EQ(0, truncate(file_rwt, 10));
+	EXPECT_EQ(0, truncate64(file_rwt, 20));
+
+	reg_fd = open(file_rwt, O_WRONLY | O_TRUNC | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	ASSERT_EQ(0, close(reg_fd));
+
+	reg_fd = open(file_rwt, O_RDONLY | O_TRUNC | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	ASSERT_EQ(0, close(reg_fd));
+
+	/* Checks read and write rights: no truncate variant works. */
+	reg_fd = open(file_rw, O_WRONLY | 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(file_rw, 10));
+	EXPECT_EQ(EACCES, errno);
+	EXPECT_EQ(-1, truncate64(file_rw, 20));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, open(file_rw, O_WRONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, open(file_rw, O_RDONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	/* Checks read and truncate right: truncate works, also with open(2). */
+	EXPECT_EQ(-1, open(file_rt, O_WRONLY | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(0, truncate(file_rt, 10));
+	EXPECT_EQ(0, truncate64(file_rt, 20));
+
+	reg_fd = open(file_rt, O_RDONLY | O_TRUNC | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	ASSERT_EQ(0, fstat(reg_fd, &statbuf));
+	EXPECT_EQ(0, statbuf.st_size);
+	ASSERT_EQ(0, close(reg_fd));
+
+	/* Checks truncate right: truncate works, but can't open file. */
+	EXPECT_EQ(-1, open(file_t, O_WRONLY | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(0, truncate(file_t, 10));
+	EXPECT_EQ(0, truncate64(file_t, 20));
+
+	EXPECT_EQ(-1, open(file_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	/* Checks "no rights" case: No form of truncation works. */
+	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, truncate(file_none, 10));
+	EXPECT_EQ(EACCES, errno);
+	EXPECT_EQ(-1, truncate64(file_none, 20));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	/* Checks truncate right on directory:  */
+	EXPECT_EQ(-1, open(file_in_dir_t, O_WRONLY | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(0, truncate(file_in_dir_t, 10));
+	EXPECT_EQ(0, truncate64(file_in_dir_t, 20));
+
+	EXPECT_EQ(-1, open(file_in_dir_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+}
+
+/*
+ * 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 *file_r = file1_s1d1;
+	const char *file_w = file2_s1d1;
+	const char *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);
+	int reg_fd;
+
+	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, truncate(file_r, 10));
+	EXPECT_EQ(0, truncate64(file_r, 20));
+
+	reg_fd = open(file_r, O_RDONLY | O_TRUNC | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	ASSERT_EQ(0, close(reg_fd));
+
+	EXPECT_EQ(-1, open(file_r, O_WRONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	/* Checks write right: truncation should work through truncate, ftruncate and open. */
+	EXPECT_EQ(0, truncate(file_w, 10));
+	EXPECT_EQ(0, truncate64(file_w, 20));
+
+	EXPECT_EQ(-1, open(file_w, O_RDONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	reg_fd = open(file_w, O_WRONLY | O_TRUNC | O_CLOEXEC);
+	ASSERT_LE(0, reg_fd);
+	ASSERT_EQ(0, close(reg_fd));
+
+	reg_fd = open(file_w, O_WRONLY);
+	ASSERT_LE(0, reg_fd);
+	EXPECT_EQ(0, ftruncate(reg_fd, 10));
+	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
+	ASSERT_EQ(0, close(reg_fd));
+
+	/* Checks "no rights" case: truncate works but all open attempts fail. */
+	EXPECT_EQ(0, truncate(file_none, 10));
+	EXPECT_EQ(0, truncate64(file_none, 20));
+
+	EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_TRUNC | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+
+	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
+	EXPECT_EQ(EACCES, errno);
+}
+
+/* Exercises creat(), which is equivalent to an open with O_CREAT|O_WRONLY|O_TRUNC. */
+TEST_F_FORK(layout1, truncate_creat)
+{
+	const struct rule rules[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{}
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_WRITE_FILE |
+			      LANDLOCK_ACCESS_FS_TRUNCATE;
+	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
+	int reg_fd;
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks creating a new file: This should work even without the truncate right. */
+	ASSERT_EQ(0, unlink(file1_s1d1));
+
+	reg_fd = creat(file1_s1d1, 0600);
+	ASSERT_LE(0, reg_fd);
+	ASSERT_EQ(0, close(reg_fd));
+
+	/*
+	 * Checks creating a file over an existing one:
+	 * This should fail. It would truncate the file, and we don't have that right.
+	 */
+	EXPECT_EQ(-1, creat(file2_s1d1, 0600));
+	EXPECT_EQ(EACCES, errno);
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.0

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

[Günther Noack]

The sample tool will restrict the truncate operation if possible with
the current kernel.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
---
 samples/landlock/sandboxer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 3e404e51ec64..4c3ed0097ffd 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,11 +161,15 @@ 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)
+	LANDLOCK_ACCESS_FS_REFER | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 #define ACCESS_ABI_2 ( \
 	LANDLOCK_ACCESS_FS_REFER)
 
+#define ACCESS_ABI_3 ( \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
+
 /* clang-format on */
 
 int main(const int argc, char *const argv[], char *const *const envp)
@@ -226,6 +231,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		return 1;
 	}
 	/* Best-effort security. */
+	if (abi < 3) {
+		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
+		access_fs_ro &= ~ACCESS_ABI_3;
+		access_fs_rw &= ~ACCESS_ABI_3;
+	}
+
 	if (abi < 2) {
 		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
 		access_fs_ro &= ~ACCESS_ABI_2;
-- 
2.37.0

[PATCH v2 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 if needed.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
---
 Documentation/userspace-api/landlock.rst | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index b86fd94ae797..41fa464cc8b8 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,14 +70,22 @@ 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 == -1) {
+        perror("Landlock is unsupported on this kernel");
+        return 1;
+    }
+    if (abi < 3) {
+        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+    }
     if (abi < 2) {
         ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
     }
@@ -127,8 +136,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
-- 
2.37.0

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

[Mickaël Salaün]

On 12/07/2022 23:14, 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>
> Link: https://lore.kernel.org/all/20220707200612.132705-2-gnoack3000@gmail.com/

Please don't include Link tags pointing to the previous version of the 
patch. You could add it after a "---" but you already added a link to 
the v1 in your cover letter, which is great. I'll add fresh links 
pointing to your emails when I'll merge your changes.

Also, don't add anything after your Signed-off-by, it should be the last 
line of your commit message.


> ---
>   Documentation/userspace-api/landlock.rst     |  5 +++++
>   include/uapi/linux/landlock.h                | 13 +++++++++----
>   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, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index b8ea59493964..b86fd94ae797 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -380,6 +380,11 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
>   Previous limitations
>   ====================
>   
> +File truncation (ABI 1-2)

I think "ABI < 3" should be easier to understand.


> +-------------------------
> +
> +File truncation was not restrictable in ABI 1-2, so it was always permitted.

What about:
File truncation couldn't be denied before the third Landlock ABI, so it 
is always allowed for kernels only supporting the first to the second ABI.


> +
>   File renaming and linking (ABI 1)
>   ---------------------------------
>   
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..9ca7f9d0d862 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -96,7 +96,12 @@ struct landlock_path_beneath_attr {
>    *
>    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
>    * - %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 open(2) using O_TRUNC or 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 truncate(2), ftruncate(2), open(2) with O_TRUNC or 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 +144,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 +164,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 v2 1/4] landlock: Support file truncation

[Günther Noack]

On Thu, Jul 28, 2022 at 06:25:52PM +0200, Mickaël Salaün wrote:
>
> On 12/07/2022 23:14, 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>
> > Link: https://lore.kernel.org/all/20220707200612.132705-2-gnoack3000@gmail.com/
>
> Please don't include Link tags pointing to the previous version of the
> patch. You could add it after a "---" but you already added a link to the v1
> in your cover letter, which is great. I'll add fresh links pointing to your
> emails when I'll merge your changes.
>
> Also, don't add anything after your Signed-off-by, it should be the last
> line of your commit message.

Thanks, noted and fixed for the next version.

>
>
> > ---
> >   Documentation/userspace-api/landlock.rst     |  5 +++++
> >   include/uapi/linux/landlock.h                | 13 +++++++++----
> >   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, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index b8ea59493964..b86fd94ae797 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> > @@ -380,6 +380,11 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
> >   Previous limitations
> >   ====================
> > +File truncation (ABI 1-2)
>
> I think "ABI < 3" should be easier to understand.

Agreed, done.

>
>
> > +-------------------------
> > +
> > +File truncation was not restrictable in ABI 1-2, so it was always permitted.
>
> What about:
> File truncation couldn't be denied before the third Landlock ABI, so it is
> always allowed for kernels only supporting the first to the second ABI.

Sounds good, done.

Will include these fixes in the next version. Thank you for the review!

--Günther

>
>
> > +
> >   File renaming and linking (ABI 1)
> >   ---------------------------------
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..9ca7f9d0d862 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -96,7 +96,12 @@ struct landlock_path_beneath_attr {
> >    *
> >    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> >    * - %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 open(2) using O_TRUNC or 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 truncate(2), ftruncate(2), open(2) with O_TRUNC or 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 +144,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 +164,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 v2 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

On 12/07/2022 23:14, Günther Noack wrote:
> These tests exercise the following truncation operations:
> 
> * truncate() and trunate64() (truncate by path)
> * ftruncate() and ftruncate64() (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>
> Link: https://lore.kernel.org/all/20220707200612.132705-3-gnoack3000@gmail.com/
> ---
>   tools/testing/selftests/landlock/fs_test.c | 238 +++++++++++++++++++++
>   1 file changed, 238 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..1e5660118bce 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3023,6 +3023,244 @@ TEST_F_FORK(layout1, proc_pipe)
>   	ASSERT_EQ(0, close(pipe_fds[1]));
>   }
>   
> +TEST_F_FORK(layout1, truncate)
> +{
> +	const char *file_rwt = file1_s1d1;

You can make file_rwt (and others) const too:
const char *const file_rwt = file1_s1d1;


> +	const char *file_rw = file2_s1d1;
> +	const char *file_rt = file1_s1d2;
> +	const char *file_t = file2_s1d2;
> +	const char *file_none = file1_s1d3;
> +	const char *dir_t = dir_s2d1;
> +	const char *file_in_dir_t = file1_s2d1;
> +	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,
> +		},
> +		{},
> +	};
> +	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);
> +	struct stat statbuf;
> +	int reg_fd;
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks read, write and truncate rights: truncate and ftruncate work. */
> +	reg_fd = open(file_rwt, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
> +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> +	ASSERT_EQ(0, close(reg_fd));

Could you add the same ftruncate* checks with a O_RDONLY FD?


> +
> +	EXPECT_EQ(0, truncate(file_rwt, 10));
> +	EXPECT_EQ(0, truncate64(file_rwt, 20));
> +
> +	reg_fd = open(file_rwt, O_WRONLY | O_TRUNC | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	reg_fd = open(file_rwt, O_RDONLY | O_TRUNC | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	ASSERT_EQ(0, close(reg_fd));

I'm not sure if it would be worth it, but can you try to move these 
checks in a standalone helper (like test_execute) that could be use for 
all/most scenarios?


> +
> +	/* Checks read and write rights: no truncate variant works. */
> +	reg_fd = open(file_rw, O_WRONLY | 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(file_rw, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, truncate64(file_rw, 20));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, open(file_rw, O_WRONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, open(file_rw, O_RDONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	/* Checks read and truncate right: truncate works, also with open(2). */

"right*s*" ? Just keep it consistent with other comments.


> +	EXPECT_EQ(-1, open(file_rt, O_WRONLY | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file_rt, 10));
> +	EXPECT_EQ(0, truncate64(file_rt, 20));
> +
> +	reg_fd = open(file_rt, O_RDONLY | O_TRUNC | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	ASSERT_EQ(0, fstat(reg_fd, &statbuf));
> +	EXPECT_EQ(0, statbuf.st_size);
> +	ASSERT_EQ(0, close(reg_fd));

Why checking fstat? And why only here?


> +
> +	/* Checks truncate right: truncate works, but can't open file. */
> +	EXPECT_EQ(-1, open(file_t, O_WRONLY | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file_t, 10));
> +	EXPECT_EQ(0, truncate64(file_t, 20));
> +
> +	EXPECT_EQ(-1, open(file_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	/* Checks "no rights" case: No form of truncation works. */

Nit: Do we need an "s" if there is none?


> +	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, truncate(file_none, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, truncate64(file_none, 20));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	/* Checks truncate right on directory:  */
> +	EXPECT_EQ(-1, open(file_in_dir_t, O_WRONLY | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file_in_dir_t, 10));
> +	EXPECT_EQ(0, truncate64(file_in_dir_t, 20));
> +
> +	EXPECT_EQ(-1, open(file_in_dir_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +}
> +
> +/*
> + * 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)

Could you merge these truncate_unhandled tests with TEST_F_FORK(layout1, 
truncate)? You can use it as a first layer of rules. This will make sure 
that nested stacking with different handled access rights works as expected.


> +{
> +	const char *file_r = file1_s1d1;
> +	const char *file_w = file2_s1d1;
> +	const char *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);
> +	int reg_fd;
> +
> +	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, truncate(file_r, 10));
> +	EXPECT_EQ(0, truncate64(file_r, 20));
> +
> +	reg_fd = open(file_r, O_RDONLY | O_TRUNC | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	EXPECT_EQ(-1, open(file_r, O_WRONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	/* Checks write right: truncation should work through truncate, ftruncate and open. */
> +	EXPECT_EQ(0, truncate(file_w, 10));
> +	EXPECT_EQ(0, truncate64(file_w, 20));
> +
> +	EXPECT_EQ(-1, open(file_w, O_RDONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	reg_fd = open(file_w, O_WRONLY | O_TRUNC | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	reg_fd = open(file_w, O_WRONLY);
> +	ASSERT_LE(0, reg_fd);
> +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
> +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> +	EXPECT_EQ(0, truncate(file_none, 10));
> +	EXPECT_EQ(0, truncate64(file_none, 20));
> +
> +	EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_TRUNC | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
> +	EXPECT_EQ(EACCES, errno);
> +}
> +
> +/* Exercises creat(), which is equivalent to an open with O_CREAT|O_WRONLY|O_TRUNC. */
> +TEST_F_FORK(layout1, truncate_creat)

You can merge these truncate_creat tests too in TEST_F_FORK(layout1, 
truncate) (without dedicated layer though).


> +{
> +	const struct rule rules[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{}
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_WRITE_FILE |
> +			      LANDLOCK_ACCESS_FS_TRUNCATE;
> +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> +	int reg_fd;
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks creating a new file: This should work even without the truncate right. */
> +	ASSERT_EQ(0, unlink(file1_s1d1));
> +
> +	reg_fd = creat(file1_s1d1, 0600);
> +	ASSERT_LE(0, reg_fd);
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	/*
> +	 * Checks creating a file over an existing one:
> +	 * This should fail. It would truncate the file, and we don't have that right.
> +	 */
> +	EXPECT_EQ(-1, creat(file2_s1d1, 0600));
> +	EXPECT_EQ(EACCES, errno);
> +}
> +
>   /* clang-format off */
>   FIXTURE(layout1_bind) {};
>   /* clang-format on */

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

[Mickaël Salaün]

On 12/07/2022 23:14, Günther Noack wrote:
> The sample tool will restrict the truncate operation if possible with
> the current kernel.

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


> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> ---
>   samples/landlock/sandboxer.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index 3e404e51ec64..4c3ed0097ffd 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,11 +161,15 @@ 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)
> +	LANDLOCK_ACCESS_FS_REFER | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   
>   #define ACCESS_ABI_2 ( \
>   	LANDLOCK_ACCESS_FS_REFER)
>   
> +#define ACCESS_ABI_3 ( \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
> +
>   /* clang-format on */
>   
>   int main(const int argc, char *const argv[], char *const *const envp)
> @@ -226,6 +231,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
>   		return 1;
>   	}
>   	/* Best-effort security. */
> +	if (abi < 3) {
> +		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
> +		access_fs_ro &= ~ACCESS_ABI_3;
> +		access_fs_rw &= ~ACCESS_ABI_3;
> +	}

I think it is a good time to start replacing this "if" check with a 
switch one:

switch (abi) {
case 1:
	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
	access_fs_rw &= ~ACCESS_ABI_2;
	__attribute__((fallthrough));
case 2:
	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
	access_fs_rw &= ~ACCESS_ABI_3;
}


> +
>   	if (abi < 2) {
>   		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
>   		access_fs_ro &= ~ACCESS_ABI_2;

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

[Mickaël Salaün]

On 29/07/2022 12:31, Mickaël Salaün wrote:
> 
> On 12/07/2022 23:14, Günther Noack wrote:
>> The sample tool will restrict the truncate operation if possible with
>> the current kernel.
> 
> "Update the sandboxer sample to restrict truncate actions.  This is
> automatically enabled by default if the running kernel supports
> LANDLOCK_ACCESS_FS_TRUNCATE, except for the paths listed in the LL_FS_RW
> environment variable."
> 
> 
>>
>> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
>> Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
>> ---
>>    samples/landlock/sandboxer.c | 15 +++++++++++++--
>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
>> index 3e404e51ec64..4c3ed0097ffd 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,11 +161,15 @@ 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)
>> +	LANDLOCK_ACCESS_FS_REFER | \
>> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>>    
>>    #define ACCESS_ABI_2 ( \
>>    	LANDLOCK_ACCESS_FS_REFER)
>>    
>> +#define ACCESS_ABI_3 ( \
>> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>> +
>>    /* clang-format on */
>>    
>>    int main(const int argc, char *const argv[], char *const *const envp)
>> @@ -226,6 +231,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
>>    		return 1;
>>    	}
>>    	/* Best-effort security. */
>> +	if (abi < 3) {
>> +		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
>> +		access_fs_ro &= ~ACCESS_ABI_3;
>> +		access_fs_rw &= ~ACCESS_ABI_3;
>> +	}
> 
> I think it is a good time to start replacing this "if" check with a
> switch one:
> 
> switch (abi) {
> case 1:
> 	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
> 	access_fs_rw &= ~ACCESS_ABI_2;
> 	__attribute__((fallthrough));
> case 2:
> 	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
> 	access_fs_rw &= ~ACCESS_ABI_3;
> }

Well, we can just mask ruleset_attr.handled_access_fs in this 
switch/case and after mask access_fs_ro and access_fs_rw after.


> 
> 
>> +
>>    	if (abi < 2) {
>>    		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
>>    		access_fs_ro &= ~ACCESS_ABI_2;

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

[Mickaël Salaün]

On 29/07/2022 12:38, Mickaël Salaün wrote:
> 
> On 29/07/2022 12:31, Mickaël Salaün wrote:
>>
>> On 12/07/2022 23:14, Günther Noack wrote:
>>> The sample tool will restrict the truncate operation if possible with
>>> the current kernel.
>>
>> "Update the sandboxer sample to restrict truncate actions.  This is
>> automatically enabled by default if the running kernel supports
>> LANDLOCK_ACCESS_FS_TRUNCATE, except for the paths listed in the LL_FS_RW
>> environment variable."
>>
>>
>>>
>>> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
>>> Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
>>> ---
>>>     samples/landlock/sandboxer.c | 15 +++++++++++++--
>>>     1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
>>> index 3e404e51ec64..4c3ed0097ffd 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,11 +161,15 @@ 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)
>>> +	LANDLOCK_ACCESS_FS_REFER | \
>>> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>>>     
>>>     #define ACCESS_ABI_2 ( \
>>>     	LANDLOCK_ACCESS_FS_REFER)
>>>     
>>> +#define ACCESS_ABI_3 ( \
>>> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>>> +
>>>     /* clang-format on */
>>>     
>>>     int main(const int argc, char *const argv[], char *const *const envp)
>>> @@ -226,6 +231,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
>>>     		return 1;
>>>     	}
>>>     	/* Best-effort security. */
>>> +	if (abi < 3) {
>>> +		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
>>> +		access_fs_ro &= ~ACCESS_ABI_3;
>>> +		access_fs_rw &= ~ACCESS_ABI_3;
>>> +	}
>>
>> I think it is a good time to start replacing this "if" check with a
>> switch one:
>>
>> switch (abi) {
>> case 1:
>> 	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
>> 	access_fs_rw &= ~ACCESS_ABI_2;
>> 	__attribute__((fallthrough));
>> case 2:
>> 	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
>> 	access_fs_rw &= ~ACCESS_ABI_3;
>> }
> 
> Well, we can just mask ruleset_attr.handled_access_fs in this
> switch/case and after mask access_fs_ro and access_fs_rw after.

In fact, we can remove ACCESS_ABI_2 and ACCESS_ABI_3 (which are only 
used here).

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

[Mickaël Salaün]

On 12/07/2022 23:14, 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 if needed.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> ---
>   Documentation/userspace-api/landlock.rst | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index b86fd94ae797..41fa464cc8b8 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,14 +70,22 @@ 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 == -1) {
> +        perror("Landlock is unsupported on this kernel");

"Landlock is not supported with the running kernel"?


> +        return 1;
> +    }
> +    if (abi < 3) {
> +        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
> +    }

I guess we could use the same switch/case code as for the sample. I'm 
not sure what would be the less confusing for users though.


>       if (abi < 2) {
>           ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
>       }
> @@ -127,8 +136,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.

Good!

>   
>   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

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

[Mickaël Salaün]

On 12/07/2022 23:14, Günther Noack wrote:

[...]

> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..9ca7f9d0d862 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -96,7 +96,12 @@ struct landlock_path_beneath_attr {
>    *
>    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
>    * - %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 open(2) using O_TRUNC or creat(2).

Please use the :manpage: notation for syscalls.


>    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation
> + *   APIs like truncate(2), ftruncate(2), open(2) with O_TRUNC or 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 +144,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 */

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

[Günther Noack]

On Fri, Jul 29, 2022 at 12:49:29PM +0200, Mickaël Salaün wrote:
>
> On 12/07/2022 23:14, Günther Noack wrote:
>
> [...]
>
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..9ca7f9d0d862 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -96,7 +96,12 @@ struct landlock_path_beneath_attr {
> >    *
> >    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> >    * - %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 open(2) using O_TRUNC or creat(2).
>
> Please use the :manpage: notation for syscalls.

Done, will be included in the next version.

>
>
> >    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> > + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation
> > + *   APIs like truncate(2), ftruncate(2), open(2) with O_TRUNC or 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 +144,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 */

--

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

[Günther Noack]

On Fri, Jul 29, 2022 at 11:39:40AM +0200, Mickaël Salaün wrote:
>
> On 12/07/2022 23:14, Günther Noack wrote:
> > These tests exercise the following truncation operations:
> >
> > * truncate() and trunate64() (truncate by path)
> > * ftruncate() and ftruncate64() (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>
> > Link: https://lore.kernel.org/all/20220707200612.132705-3-gnoack3000@gmail.com/
> > ---
> >   tools/testing/selftests/landlock/fs_test.c | 238 +++++++++++++++++++++
> >   1 file changed, 238 insertions(+)
> >
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index cb77eaa01c91..1e5660118bce 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -3023,6 +3023,244 @@ TEST_F_FORK(layout1, proc_pipe)
> >   	ASSERT_EQ(0, close(pipe_fds[1]));
> >   }
> > +TEST_F_FORK(layout1, truncate)
> > +{
> > +	const char *file_rwt = file1_s1d1;
>
> You can make file_rwt (and others) const too:
> const char *const file_rwt = file1_s1d1;

Done.

>
>
> > +	const char *file_rw = file2_s1d1;
> > +	const char *file_rt = file1_s1d2;
> > +	const char *file_t = file2_s1d2;
> > +	const char *file_none = file1_s1d3;
> > +	const char *dir_t = dir_s2d1;
> > +	const char *file_in_dir_t = file1_s2d1;
> > +	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,
> > +		},
> > +		{},
> > +	};
> > +	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);
> > +	struct stat statbuf;
> > +	int reg_fd;
> > +
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Checks read, write and truncate rights: truncate and ftruncate work. */
> > +	reg_fd = open(file_rwt, O_WRONLY | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
> > +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> > +	ASSERT_EQ(0, close(reg_fd));
>
> Could you add the same ftruncate* checks with a O_RDONLY FD?

Done.

Side note: ftruncate(2) on a read-only FD returns EINVAL because the
FD must be writable. (This happens independent of LSM policies.)

>
>
> > +
> > +	EXPECT_EQ(0, truncate(file_rwt, 10));
> > +	EXPECT_EQ(0, truncate64(file_rwt, 20));
> > +
> > +	reg_fd = open(file_rwt, O_WRONLY | O_TRUNC | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	reg_fd = open(file_rwt, O_RDONLY | O_TRUNC | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	ASSERT_EQ(0, close(reg_fd));
>
> I'm not sure if it would be worth it, but can you try to move these checks
> in a standalone helper (like test_execute) that could be use for all/most
> scenarios?

Thanks for the suggestion, this has simplified the tests a lot!

I created additional helpers test_truncate(), test_ftruncate() and
test_creat() which call the corresponding syscalls, do necessary
file-descriptor cleanups and which just return the errno or 0, as it
was already done by test_open().

So the tests are now basically just a sequence of

  EXPECT_EQ(EACCES, test_something(...));

calls, with differing operations exercised and expected errors.

>
>
> > +
> > +	/* Checks read and write rights: no truncate variant works. */
> > +	reg_fd = open(file_rw, O_WRONLY | 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(file_rw, 10));
> > +	EXPECT_EQ(EACCES, errno);
> > +	EXPECT_EQ(-1, truncate64(file_rw, 20));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, open(file_rw, O_WRONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, open(file_rw, O_RDONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	/* Checks read and truncate right: truncate works, also with open(2). */
>
> "right*s*" ? Just keep it consistent with other comments.

Done.

>
>
> > +	EXPECT_EQ(-1, open(file_rt, O_WRONLY | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(0, truncate(file_rt, 10));
> > +	EXPECT_EQ(0, truncate64(file_rt, 20));
> > +
> > +	reg_fd = open(file_rt, O_RDONLY | O_TRUNC | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	ASSERT_EQ(0, fstat(reg_fd, &statbuf));
> > +	EXPECT_EQ(0, statbuf.st_size);
> > +	ASSERT_EQ(0, close(reg_fd));
>
> Why checking fstat? And why only here?

Fixed; you are right, this was a bit inconsistent and just made it more confusing.

>
>
> > +
> > +	/* Checks truncate right: truncate works, but can't open file. */
> > +	EXPECT_EQ(-1, open(file_t, O_WRONLY | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(0, truncate(file_t, 10));
> > +	EXPECT_EQ(0, truncate64(file_t, 20));
> > +
> > +	EXPECT_EQ(-1, open(file_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	/* Checks "no rights" case: No form of truncation works. */
>
> Nit: Do we need an "s" if there is none?

I believe plural is correct after "no", as in "no dogs allowed".

>
>
> > +	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, truncate(file_none, 10));
> > +	EXPECT_EQ(EACCES, errno);
> > +	EXPECT_EQ(-1, truncate64(file_none, 20));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	/* Checks truncate right on directory:  */
> > +	EXPECT_EQ(-1, open(file_in_dir_t, O_WRONLY | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(0, truncate(file_in_dir_t, 10));
> > +	EXPECT_EQ(0, truncate64(file_in_dir_t, 20));
> > +
> > +	EXPECT_EQ(-1, open(file_in_dir_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +}
> > +
> > +/*
> > + * 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)
>
> Could you merge these truncate_unhandled tests with TEST_F_FORK(layout1,
> truncate)? You can use it as a first layer of rules. This will make sure
> that nested stacking with different handled access rights works as expected.

I have attempted this, but the layering of rules quickly made it
difficult to reason about the test (it's more difficult to track which
files has which rights). I would prefer to test these concerns
separate from each other, so that each test exercises a less
complicated scenario and is easier to reason about.

I see that layering is already exercised in other tests in fs_test.c.
I imagine that the kernel code for that works the same for the entire
bitmask, so testing it for truncate specifically probably wouldn't
give us that much additional confidence?

Please let me know if you feel strongly about it, in case I'm
overlooking a complication.

>
>
> > +{
> > +	const char *file_r = file1_s1d1;
> > +	const char *file_w = file2_s1d1;
> > +	const char *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);
> > +	int reg_fd;
> > +
> > +	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, truncate(file_r, 10));
> > +	EXPECT_EQ(0, truncate64(file_r, 20));
> > +
> > +	reg_fd = open(file_r, O_RDONLY | O_TRUNC | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	EXPECT_EQ(-1, open(file_r, O_WRONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	/* Checks write right: truncation should work through truncate, ftruncate and open. */
> > +	EXPECT_EQ(0, truncate(file_w, 10));
> > +	EXPECT_EQ(0, truncate64(file_w, 20));
> > +
> > +	EXPECT_EQ(-1, open(file_w, O_RDONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	reg_fd = open(file_w, O_WRONLY | O_TRUNC | O_CLOEXEC);
> > +	ASSERT_LE(0, reg_fd);
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	reg_fd = open(file_w, O_WRONLY);
> > +	ASSERT_LE(0, reg_fd);
> > +	EXPECT_EQ(0, ftruncate(reg_fd, 10));
> > +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> > +	EXPECT_EQ(0, truncate(file_none, 10));
> > +	EXPECT_EQ(0, truncate64(file_none, 20));
> > +
> > +	EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_TRUNC | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +
> > +	EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
> > +	EXPECT_EQ(EACCES, errno);
> > +}
> > +
> > +/* Exercises creat(), which is equivalent to an open with O_CREAT|O_WRONLY|O_TRUNC. */
> > +TEST_F_FORK(layout1, truncate_creat)
>
> You can merge these truncate_creat tests too in TEST_F_FORK(layout1,
> truncate) (without dedicated layer though).

Done.

I added an additional file f1 under s3d1 in layout1, so that it would
be easy to exercise. (The creat tests rely on the rights being given
on the parent directory, because they unlink the file at some point.)

>
>
> > +{
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = dir_s1d1,
> > +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +		},
> > +		{}
> > +	};
> > +	const __u64 handled = LANDLOCK_ACCESS_FS_WRITE_FILE |
> > +			      LANDLOCK_ACCESS_FS_TRUNCATE;
> > +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> > +	int reg_fd;
> > +
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Checks creating a new file: This should work even without the truncate right. */
> > +	ASSERT_EQ(0, unlink(file1_s1d1));
> > +
> > +	reg_fd = creat(file1_s1d1, 0600);
> > +	ASSERT_LE(0, reg_fd);
> > +	ASSERT_EQ(0, close(reg_fd));
> > +
> > +	/*
> > +	 * Checks creating a file over an existing one:
> > +	 * This should fail. It would truncate the file, and we don't have that right.
> > +	 */
> > +	EXPECT_EQ(-1, creat(file2_s1d1, 0600));
> > +	EXPECT_EQ(EACCES, errno);
> > +}
> > +
> >   /* clang-format off */
> >   FIXTURE(layout1_bind) {};
> >   /* clang-format on */

--

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

[Günther Noack]

On Fri, Jul 29, 2022 at 12:43:47PM +0200, Mickaël Salaün wrote:
>
> On 29/07/2022 12:38, Mickaël Salaün wrote:
> >
> > On 29/07/2022 12:31, Mickaël Salaün wrote:
> > >
> > > On 12/07/2022 23:14, Günther Noack wrote:
> > > > The sample tool will restrict the truncate operation if possible with
> > > > the current kernel.
> > >
> > > "Update the sandboxer sample to restrict truncate actions.  This is
> > > automatically enabled by default if the running kernel supports
> > > LANDLOCK_ACCESS_FS_TRUNCATE, except for the paths listed in the LL_FS_RW
> > > environment variable."

Done.

> > >
> > >
> > > >
> > > > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > > > Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> > > > ---
> > > >     samples/landlock/sandboxer.c | 15 +++++++++++++--
> > > >     1 file changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> > > > index 3e404e51ec64..4c3ed0097ffd 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,11 +161,15 @@ 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)
> > > > +	LANDLOCK_ACCESS_FS_REFER | \
> > > > +	LANDLOCK_ACCESS_FS_TRUNCATE)
> > > >     #define ACCESS_ABI_2 ( \
> > > >     	LANDLOCK_ACCESS_FS_REFER)
> > > > +#define ACCESS_ABI_3 ( \
> > > > +	LANDLOCK_ACCESS_FS_TRUNCATE)
> > > > +
> > > >     /* clang-format on */
> > > >     int main(const int argc, char *const argv[], char *const *const envp)
> > > > @@ -226,6 +231,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > > >     		return 1;
> > > >     	}
> > > >     	/* Best-effort security. */
> > > > +	if (abi < 3) {
> > > > +		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
> > > > +		access_fs_ro &= ~ACCESS_ABI_3;
> > > > +		access_fs_rw &= ~ACCESS_ABI_3;
> > > > +	}
> > >
> > > I think it is a good time to start replacing this "if" check with a
> > > switch one:
> > >
> > > switch (abi) {
> > > case 1:
> > > 	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
> > > 	access_fs_rw &= ~ACCESS_ABI_2;
> > > 	__attribute__((fallthrough));
> > > case 2:
> > > 	ruleset_attr.handled_access_fs &= ~ACCESS_ABI_3;
> > > 	access_fs_rw &= ~ACCESS_ABI_3;
> > > }
> >
> > Well, we can just mask ruleset_attr.handled_access_fs in this
> > switch/case and after mask access_fs_ro and access_fs_rw after.
>
> In fact, we can remove ACCESS_ABI_2 and ACCESS_ABI_3 (which are only used
> here).

Done (all three).

--

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

[Günther Noack]

On Fri, Jul 29, 2022 at 12:47:46PM +0200, Mickaël Salaün wrote:
>
> On 12/07/2022 23:14, 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 if needed.
> >
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > Link: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> > ---
> >   Documentation/userspace-api/landlock.rst | 19 ++++++++++++++-----
> >   1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index b86fd94ae797..41fa464cc8b8 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,14 +70,22 @@ 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 == -1) {
> > +        perror("Landlock is unsupported on this kernel");
>
> "Landlock is not supported with the running kernel"?

Done.

>
>
> > +        return 1;
> > +    }
> > +    if (abi < 3) {
> > +        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
> > +    }
>
> I guess we could use the same switch/case code as for the sample. I'm not
> sure what would be the less confusing for users though.

Done. (Both are mildly confusing, IMHO %-))

> [...]

--