[PATCH v8 0/9] landlock: truncate support

[PATCH v8 0/9] 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.

Motivation
----------

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.

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.

Implementation overview
-----------------------

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.

Additionally, this patch set introduces a new Landlock security blob
associated with opened files, to track the available Landlock access
rights at the time of opening the file. This is in line with Unix's
general approach of checking the read and write permissions during
open(), and associating this previously checked authorization with the
opened file.

In order to treat truncate(2) and ftruncate(2) calls differently in an
LSM hook, we split apart the existing security_path_truncate hook into
security_path_truncate (for truncation by path) and
security_file_truncate (for truncation of previously opened files).

Relationship between "truncate" and "write" rights
--------------------------------------------------

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.

When the "truncate" right is checked for ftruncate(2)
-----------------------------------------------------

Notably, for the purpose of ftruncate(2), the Landlock truncation
access right is looked up when *opening* the file, not when calling
ftruncate(). The availability of the truncate right is associated with
the opened file and is later checked to authorize ftruncate(2)
operations.

This is similar to how the write mode gets remembered after a
open(..., O_WRONLY) to authorize later write() operations.

These opened file descriptors can also be passed between processes and
will continue to enforce their truncation properties when these
processes attempt an ftruncate().

Ongoing discussions
-------------------

The one remaining ongoing discussion from v6 of the patch set is the
question whether we need to touch fs/ksmbd and fs/cachefiles, which
are both using vfs_truncate() to truncate files by path, even though
they already have the same struct file open. The proposal was to
introduce a "vfs_ftruncate()" that would work on opened files.

I think we should decouple this from the truncate patch set, with the
reasoning that:

(a) it would be a bigger change to create a "vfs_ftruncate()" which
would reach beyond the scope of this patch set.

(b) it seems likely that both components do not need to run under
Landlock at the moment and can be updated independently (just like it
needs to happen for normal userspace software in order to run it under
Landlock).

(c) vfs_truncate() is not the perfectly narrowest API for truncating
an opened file, but it's a legitimate way to do that and the operation
*is* checked with a Landlock LSM hook, although it might potentially
permit for a narrower sandboxing if that was done differently. That's
speculative though.

Overall, it's unclear whether doing this has any sandboxing benefits
for ksmbd and cachefiles, whereas on the downside, it would expand the
scope of the patch set quite a bit and would have to touch core parts
of the kernel (fs/open.c).

These patches are based on version 6.0-rc7.

Best regards,
Günther

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

Past discussions:
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/
V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
V5: https://lore.kernel.org/all/20220817203006.21769-1-gnoack3000@gmail.com/
V6: https://lore.kernel.org/all/20220908195805.128252-1-gnoack3000@gmail.com/
V7: https://lore.kernel.org/all/20220930160144.141504-1-gnoack3000@gmail.com/

Changelog:

V8:
* landlock: Refactor check_access_path_dual() into
  is_access_to_paths_allowed(), as suggested by Mickaël Salaün on the
  v7 review. Added this as a separate commit.
* landlock truncate feature: inline get_path_access()
* Documentation: update documentation date to October 2022
* selftests: locally #define __maybe_unused (checkpatch started
  complaining about it, but the header where it's defined is not
  usable from selftests)

V7:
* security: Create file_truncate hook
  * Fix the build on kernels without CONFIG_SECURITY_PATH (fixed by
    Mickaël Salaün)
  * lsm_hooks.h: Document file_truncate hook
  * fs/open.c: undo accidental indentation changes
* landlock: Support file truncation
  * Use the init_layer_masks() result as input for
    check_access_path_dual()
  * Naming
    * Rename the landlock_file_security.allowed_access field
      (previously called "rights")
    * Rename get_path_access_rights() to get_path_access()
    * Rename get_file_access() to get_required_file_open_access() to
      avoid confusion with get_path_access()
    * Use "access_request" for access_mask_t variables, access_req for
      unsigned long
  * Documentation:
    * Fixed some comments according to review
    * Added comments to get_required_file_open_access() and
      init_layer_masks()
* selftests:
  * remove unused variables
  * rename fd0,...,fd3 to fd_layer0,...,fd_layer3.
  * test_ftruncate: define layers on top and inline the helper function
* New tests (both added as separate commits)
  * More exhaustive ftruncate test: Add open_and_ftruncate test that
    exercises ftruncate more thoroughly with fixture variants
  * FD-passing test: exercise restricted file descriptors getting
    passed between processes, also using the same fixture variants
* Documentation: integrate review comments by Mickaël Salaün
  * do not use contraptions (don't)
  * use double backquotes in all touched lines
  * add the read/write open() analogy to the truncation docs
  * in code example, check for abi<0 explicitly and fix indentation

V6:
* LSM hooks: create file_truncate hook in addition to path_truncate.
  Use it in the existing path_truncate call sites where appropriate.
* landlock: check LANDLOCK_ACCESS_FS_TRUNCATE right during open(), and
  associate that right with the opened struct file in a security blob.
  Introduce get_path_access_rights() helper function.
* selftests: test ftruncate in a separate test, to exercise that
  the rights are associated with the file descriptor.
* Documentation: Rework documentation to reflect new ftruncate() semantics.
* Applied small fixes by Mickaël Salaün which he added on top of V5, in
  https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
  (I hope I found them all.)

V5:
* Documentation
  * Fix wording in userspace-api headers and in landlock.rst.
  * Move the truncation limitation section one to the bottom.
  * Move all .rst changes into the documentation commit.
* selftests
  * Remove _metadata argument from helpers where it became unnecessary.
  * Open writable file descriptors at the top of both tests, before Landlock
    is enabled, to exercise ftruncate() independently from open().
  * Simplify test_ftruncate and decouple it from exercising open().
  * test_creat(): Return errno on close() failure (it does not conflict).
  * Fix /* comment style */
  * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
  * Add missing |O_TRUNC to a check in one test.
  * Put the truncate_unhandled test before the other.

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.

Günther Noack (9):
  security: Create file_truncate hook from path_truncate hook
  selftests/landlock: Locally define __maybe_unused
  landlock: Refactor check_access_path_dual() into
    is_access_to_paths_allowed()
  landlock: Support file truncation
  selftests/landlock: Selftests for file truncation support
  selftests/landlock: Test open() and ftruncate() in multiple scenarios
  selftests/landlock: Test FD passing from a Landlock-restricted to an
    unrestricted process
  samples/landlock: Extend sample tool to support
    LANDLOCK_ACCESS_FS_TRUNCATE
  landlock: Document Landlock's file truncation support

 Documentation/userspace-api/landlock.rst     |  66 ++-
 fs/namei.c                                   |   2 +-
 fs/open.c                                    |   2 +-
 include/linux/lsm_hook_defs.h                |   1 +
 include/linux/lsm_hooks.h                    |  10 +-
 include/linux/security.h                     |   6 +
 include/uapi/linux/landlock.h                |  21 +-
 samples/landlock/sandboxer.c                 |  23 +-
 security/apparmor/lsm.c                      |   6 +
 security/landlock/fs.c                       | 191 +++++---
 security/landlock/fs.h                       |  24 +
 security/landlock/limits.h                   |   2 +-
 security/landlock/setup.c                    |   1 +
 security/landlock/syscalls.c                 |   2 +-
 security/security.c                          |   5 +
 security/tomoyo/tomoyo.c                     |  13 +
 tools/testing/selftests/landlock/base_test.c |  38 +-
 tools/testing/selftests/landlock/common.h    |  85 +++-
 tools/testing/selftests/landlock/fs_test.c   | 452 ++++++++++++++++++-
 19 files changed, 828 insertions(+), 122 deletions(-)


base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
-- 
2.37.3

[PATCH v8 1/9] security: Create file_truncate hook from path_truncate hook

[Günther Noack]

Like path_truncate, the file_truncate hook also restricts file
truncation, but is called in the cases where truncation is attempted
on an already-opened file.

This is required in a subsequent commit to handle ftruncate()
operations differently to truncate() operations.

Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 fs/namei.c                    |  2 +-
 fs/open.c                     |  2 +-
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/lsm_hooks.h     | 10 +++++++++-
 include/linux/security.h      |  6 ++++++
 security/apparmor/lsm.c       |  6 ++++++
 security/security.c           |  5 +++++
 security/tomoyo/tomoyo.c      | 13 +++++++++++++
 8 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 53b4bc094db2..0e419bd30f8e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
 	if (error)
 		return error;
 
-	error = security_path_truncate(path);
+	error = security_file_truncate(filp);
 	if (!error) {
 		error = do_truncate(mnt_userns, path->dentry, 0,
 				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
diff --git a/fs/open.c b/fs/open.c
index cf7e5c350a54..0fa861873245 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 	if (IS_APPEND(file_inode(f.file)))
 		goto out_putf;
 	sb_start_write(inode->i_sb);
-	error = security_path_truncate(&f.file->f_path);
+	error = security_file_truncate(f.file);
 	if (!error)
 		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
 				    ATTR_MTIME | ATTR_CTIME, f.file);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 60fff133c0b1..dee35ab253ba 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
 	 struct fown_struct *fown, int sig)
 LSM_HOOK(int, 0, file_receive, struct file *file)
 LSM_HOOK(int, 0, file_open, struct file *file)
+LSM_HOOK(int, 0, file_truncate, struct file *file)
 LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
 	 unsigned long clone_flags)
 LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 3aa6030302f5..4acc975f28d9 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -409,7 +409,9 @@
  *	@attr is the iattr structure containing the new file attributes.
  *	Return 0 if permission is granted.
  * @path_truncate:
- *	Check permission before truncating a file.
+ *	Check permission before truncating the file indicated by path.
+ *      Note that truncation permissions may also be checked based on
+ *      already opened files, using the @file_truncate hook.
  *	@path contains the path structure for the file.
  *	Return 0 if permission is granted.
  * @inode_getattr:
@@ -598,6 +600,12 @@
  *	to receive an open file descriptor via socket IPC.
  *	@file contains the file structure being received.
  *	Return 0 if permission is granted.
+ * @file_truncate:
+ *	Check permission before truncating a file, i.e. using ftruncate.
+ *	Note that truncation permission may also be checked based on the path,
+ *      using the @path_truncate hook.
+ *	@file contains the file structure for the file.
+ *	Return 0 if permission is granted.
  * @file_open:
  *	Save open-time permission checking state for later use upon
  *	file_permission, and recheck access if anything has changed
diff --git a/include/linux/security.h b/include/linux/security.h
index 7bd0c490703d..f80b23382dd9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
 				 struct fown_struct *fown, int sig);
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file);
+int security_file_truncate(struct file *file);
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
 void security_task_free(struct task_struct *task);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
@@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
 	return 0;
 }
 
+static inline int security_file_truncate(struct file *file)
+{
+	return 0;
+}
+
 static inline int security_task_alloc(struct task_struct *task,
 				      unsigned long clone_flags)
 {
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index e29cade7b662..98ecb7f221b8 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
 	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
 }
 
+static int apparmor_file_truncate(struct file *file)
+{
+	return apparmor_path_truncate(&file->f_path);
+}
+
 static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
 				 const char *old_name)
 {
@@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
 	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
 	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
+	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
 
 	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
 	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
diff --git a/security/security.c b/security/security.c
index 4b95de24bc8d..d73e423005c3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1650,6 +1650,11 @@ int security_file_open(struct file *file)
 	return fsnotify_perm(file, MAY_OPEN);
 }
 
+int security_file_truncate(struct file *file)
+{
+	return call_int_hook(file_truncate, 0, file);
+}
+
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
 {
 	int rc = lsm_task_alloc(task);
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 71e82d855ebf..af04a7b7eb28 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
 	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
 }
 
+/**
+ * tomoyo_file_truncate - Target for security_file_truncate().
+ *
+ * @file: Pointer to "struct file".
+ *
+ * Returns 0 on success, negative value otherwise.
+ */
+static int tomoyo_file_truncate(struct file *file)
+{
+	return tomoyo_path_truncate(&file->f_path);
+}
+
 /**
  * tomoyo_path_unlink - Target for security_path_unlink().
  *
@@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
 	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
 	LSM_HOOK_INIT(file_open, tomoyo_file_open),
+	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
 	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
 	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
 	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),
-- 
2.37.3

[PATCH v8 2/9] selftests/landlock: Locally define __maybe_unused

[Günther Noack]

The checkpatch tool started to flag __attribute__(__unused__), which
we previously used. The header where this is normally defined is not
currently compatible with selftests.

This is the same approach as used in selftests/net/psock_lib.h.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 tools/testing/selftests/landlock/common.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 7ba18eb23783..7d34592471db 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -17,6 +17,10 @@
 
 #include "../kselftest_harness.h"
 
+#ifndef __maybe_unused
+#define __maybe_unused __attribute__((__unused__))
+#endif
+
 /*
  * TEST_F_FORK() is useful when a test drop privileges but the corresponding
  * FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
@@ -140,14 +144,12 @@ static void _init_caps(struct __test_metadata *const _metadata, bool drop_all)
 }
 
 /* We cannot put such helpers in a library because of kselftest_harness.h . */
-__attribute__((__unused__)) static void
-disable_caps(struct __test_metadata *const _metadata)
+static void __maybe_unused disable_caps(struct __test_metadata *const _metadata)
 {
 	_init_caps(_metadata, false);
 }
 
-__attribute__((__unused__)) static void
-drop_caps(struct __test_metadata *const _metadata)
+static void __maybe_unused drop_caps(struct __test_metadata *const _metadata)
 {
 	_init_caps(_metadata, true);
 }
@@ -176,14 +178,14 @@ static void _effective_cap(struct __test_metadata *const _metadata,
 	}
 }
 
-__attribute__((__unused__)) static void
-set_cap(struct __test_metadata *const _metadata, const cap_value_t caps)
+static void __maybe_unused set_cap(struct __test_metadata *const _metadata,
+				   const cap_value_t caps)
 {
 	_effective_cap(_metadata, caps, CAP_SET);
 }
 
-__attribute__((__unused__)) static void
-clear_cap(struct __test_metadata *const _metadata, const cap_value_t caps)
+static void __maybe_unused clear_cap(struct __test_metadata *const _metadata,
+				     const cap_value_t caps)
 {
 	_effective_cap(_metadata, caps, CAP_CLEAR);
 }
-- 
2.37.3

[PATCH v8 3/9] landlock: Refactor check_access_path_dual() into is_access_to_paths_allowed()

[Günther Noack]

* Rename it to is_access_to_paths_allowed()
* Make it return true iff the access is allowed
* Calculate the EXDEV/EACCES error code in the one place where it's needed

Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 security/landlock/fs.c | 89 +++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 45 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index a9dbd99d9ee7..083dd3d359de 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -430,7 +430,7 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
 }
 
 /**
- * check_access_path_dual - Check accesses for requests with a common path
+ * is_access_to_paths_allowed - Check accesses for requests with a common path
  *
  * @domain: Domain to check against.
  * @path: File hierarchy to walk through.
@@ -465,14 +465,10 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
  * allow the request.
  *
  * Returns:
- * - 0 if the access request is granted;
- * - -EACCES if it is denied because of access right other than
- *   LANDLOCK_ACCESS_FS_REFER;
- * - -EXDEV if the renaming or linking would be a privileged escalation
- *   (according to each layered policies), or if LANDLOCK_ACCESS_FS_REFER is
- *   not allowed by the source or the destination.
+ * - true if the access request is granted;
+ * - false otherwise
  */
-static int check_access_path_dual(
+static bool is_access_to_paths_allowed(
 	const struct landlock_ruleset *const domain,
 	const struct path *const path,
 	const access_mask_t access_request_parent1,
@@ -492,17 +488,17 @@ static int check_access_path_dual(
 	(*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL;
 
 	if (!access_request_parent1 && !access_request_parent2)
-		return 0;
+		return true;
 	if (WARN_ON_ONCE(!domain || !path))
-		return 0;
+		return true;
 	if (is_nouser_or_private(path->dentry))
-		return 0;
+		return true;
 	if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1))
-		return -EACCES;
+		return false;
 
 	if (unlikely(layer_masks_parent2)) {
 		if (WARN_ON_ONCE(!dentry_child1))
-			return -EACCES;
+			return false;
 		/*
 		 * For a double request, first check for potential privilege
 		 * escalation by looking at domain handled accesses (which are
@@ -513,7 +509,7 @@ static int check_access_path_dual(
 		is_dom_check = true;
 	} else {
 		if (WARN_ON_ONCE(dentry_child1 || dentry_child2))
-			return -EACCES;
+			return false;
 		/* For a simple request, only check for requested accesses. */
 		access_masked_parent1 = access_request_parent1;
 		access_masked_parent2 = access_request_parent2;
@@ -622,24 +618,7 @@ static int check_access_path_dual(
 	}
 	path_put(&walker_path);
 
-	if (allowed_parent1 && allowed_parent2)
-		return 0;
-
-	/*
-	 * This prioritizes EACCES over EXDEV for all actions, including
-	 * renames with RENAME_EXCHANGE.
-	 */
-	if (likely(is_eacces(layer_masks_parent1, access_request_parent1) ||
-		   is_eacces(layer_masks_parent2, access_request_parent2)))
-		return -EACCES;
-
-	/*
-	 * Gracefully forbids reparenting if the destination directory
-	 * hierarchy is not a superset of restrictions of the source directory
-	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
-	 * source or the destination.
-	 */
-	return -EXDEV;
+	return allowed_parent1 && allowed_parent2;
 }
 
 static inline int check_access_path(const struct landlock_ruleset *const domain,
@@ -649,8 +628,10 @@ static inline int check_access_path(const struct landlock_ruleset *const domain,
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
 
 	access_request = init_layer_masks(domain, access_request, &layer_masks);
-	return check_access_path_dual(domain, path, access_request,
-				      &layer_masks, NULL, 0, NULL, NULL);
+	if (is_access_to_paths_allowed(domain, path, access_request,
+				       &layer_masks, NULL, 0, NULL, NULL))
+		return 0;
+	return -EACCES;
 }
 
 static inline int current_check_access_path(const struct path *const path,
@@ -711,8 +692,9 @@ static inline access_mask_t maybe_remove(const struct dentry *const dentry)
  * file.  While walking from @dir to @mnt_root, we record all the domain's
  * allowed accesses in @layer_masks_dom.
  *
- * This is similar to check_access_path_dual() but much simpler because it only
- * handles walking on the same mount point and only check one set of accesses.
+ * This is similar to is_access_to_paths_allowed() but much simpler because it
+ * only handles walking on the same mount point and only checks one set of
+ * accesses.
  *
  * Returns:
  * - true if all the domain access rights are allowed for @dir;
@@ -857,10 +839,11 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 		access_request_parent1 = init_layer_masks(
 			dom, access_request_parent1 | access_request_parent2,
 			&layer_masks_parent1);
-		return check_access_path_dual(dom, new_dir,
-					      access_request_parent1,
-					      &layer_masks_parent1, NULL, 0,
-					      NULL, NULL);
+		if (is_access_to_paths_allowed(
+			    dom, new_dir, access_request_parent1,
+			    &layer_masks_parent1, NULL, 0, NULL, NULL))
+			return 0;
+		return -EACCES;
 	}
 
 	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
@@ -886,11 +869,27 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	 * parent access rights.  This will be useful to compare with the
 	 * destination parent access rights.
 	 */
-	return check_access_path_dual(dom, &mnt_dir, access_request_parent1,
-				      &layer_masks_parent1, old_dentry,
-				      access_request_parent2,
-				      &layer_masks_parent2,
-				      exchange ? new_dentry : NULL);
+	if (is_access_to_paths_allowed(
+		    dom, &mnt_dir, access_request_parent1, &layer_masks_parent1,
+		    old_dentry, access_request_parent2, &layer_masks_parent2,
+		    exchange ? new_dentry : NULL))
+		return 0;
+
+	/*
+	 * This prioritizes EACCES over EXDEV for all actions, including
+	 * renames with RENAME_EXCHANGE.
+	 */
+	if (likely(is_eacces(&layer_masks_parent1, access_request_parent1) ||
+		   is_eacces(&layer_masks_parent2, access_request_parent2)))
+		return -EACCES;
+
+	/*
+	 * Gracefully forbids reparenting if the destination directory
+	 * hierarchy is not a superset of restrictions of the source directory
+	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
+	 * source or the destination.
+	 */
+	return -EXDEV;
 }
 
 /* Inode hooks */
-- 
2.37.3

[PATCH v8 4/9] 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 updates code documentation to document
the flag.

The following operations are restricted:

open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
implicitly truncated as part of the open() (e.g. using O_TRUNC).

Notable special cases:
* open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
* open() with O_TRUNC does *not* need the TRUNCATE right when it
  creates a new file.

truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
right.

ftruncate() (on a file): requires that the file had the TRUNCATE right
when it was previously opened.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 include/uapi/linux/landlock.h                |  21 +++-
 security/landlock/fs.c                       | 102 +++++++++++++++++--
 security/landlock/fs.h                       |  24 +++++
 security/landlock/limits.h                   |   2 +-
 security/landlock/setup.c                    |   1 +
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   7 +-
 8 files changed, 144 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 23df4e0e8ace..d830cdfdbe56 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -95,8 +95,19 @@ 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 with :manpage:`truncate(2)`,
+ *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
+ *   `O_TRUNC`. Whether an opened file can be truncated with
+ *   :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
+ *   same way as read and write permissions are checked during
+ *   :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
+ *   %LANDLOCK_ACCESS_FS_WRITE_FILE. 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 +150,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 +170,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 083dd3d359de..80d507ce2305 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 */
 
 /*
@@ -297,6 +298,18 @@ get_handled_accesses(const struct landlock_ruleset *const domain)
 	return access_dom & LANDLOCK_MASK_ACCESS_FS;
 }
 
+/*
+ * init_layer_masks - Populates @layer_masks such that for each access right in
+ * @access_request, the bits for all the layers are set where this access right
+ * is handled.
+ *
+ * @domain: The domain that defines the current restrictions.
+ * @access_request: The requested access rights to check.
+ * @layer_masks: The layer masks to populate.
+ *
+ * Returns: An access mask where each access right bit is set which is handled
+ * in any of the active layers in @domain.
+ */
 static inline access_mask_t
 init_layer_masks(const struct landlock_ruleset *const domain,
 		 const access_mask_t access_request,
@@ -1141,9 +1154,19 @@ 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)
+/*
+ * get_required_file_open_access - Returns the access rights that are required
+ * for opening the file, depending on the file type and open mode.
+ */
+static inline access_mask_t
+get_required_file_open_access(const struct file *const file)
 {
 	access_mask_t access = 0;
 
@@ -1163,17 +1186,82 @@ static inline access_mask_t get_file_access(const struct file *const file)
 
 static int hook_file_open(struct file *const file)
 {
+	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
+	access_mask_t open_access_request, full_access_request, allowed_access;
+	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
 	const struct landlock_ruleset *const dom =
 		landlock_get_current_domain();
 
-	if (!dom)
+	if (!dom) {
+		/*
+		 * Grants all access rights, even if most of them are not
+		 * checked later on. It is more consistent.
+		 */
+		landlock_file(file)->allowed_access = LANDLOCK_MASK_ACCESS_FS;
 		return 0;
+	}
+
 	/*
-	 * Because a file may be opened with O_PATH, get_file_access() may
-	 * return 0.  This case will be handled with a future Landlock
+	 * Because a file may be opened with O_PATH, get_required_file_open_access()
+	 * may return 0.  This case will be handled with a future Landlock
 	 * evolution.
 	 */
-	return check_access_path(dom, &file->f_path, get_file_access(file));
+	open_access_request = get_required_file_open_access(file);
+
+	/*
+	 * We look up more access than what we immediately need for open(), so
+	 * that we can later authorize operations on opened files.
+	 */
+	full_access_request = open_access_request | optional_access;
+
+	allowed_access = full_access_request;
+	if (!is_access_to_paths_allowed(
+		    dom, &file->f_path,
+		    init_layer_masks(dom, full_access_request, &layer_masks),
+		    &layer_masks, NULL, 0, NULL, NULL)) {
+		unsigned long access_bit;
+		unsigned long access_req = full_access_request;
+
+		/*
+		 * Calculate the actual allowed access rights from layer_masks.
+		 * Remove each access right from allowed_access which has been
+		 * vetoed by any layer.
+		 */
+		for_each_set_bit(access_bit, &access_req,
+				 ARRAY_SIZE(layer_masks)) {
+			if (layer_masks[access_bit])
+				allowed_access &= ~BIT_ULL(access_bit);
+		}
+	}
+
+	if (open_access_request & ~allowed_access)
+		return -EACCES;
+
+	/*
+	 * For operations on already opened files (i.e. ftruncate()), it is the
+	 * access rights at the time of open() which decide whether the
+	 * operation is permitted. Therefore, we record the relevant subset of
+	 * file access rights in the opened struct file.
+	 */
+	landlock_file(file)->allowed_access = allowed_access;
+	return 0;
+}
+
+static int hook_file_truncate(struct file *const file)
+{
+	/*
+	 * Allows truncation if the truncate right was available at the time of
+	 * opening the file, to get a consistent access check as for read, write
+	 * and execute operations.
+	 *
+	 * Note: For checks done based on the file's Landlock rights, we enforce
+	 * them independently of whether the current thread is in a Landlock
+	 * domain, so that open files passed between independent processes
+	 * retain their behaviour.
+	 */
+	if (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_TRUNCATE)
+		return 0;
+	return -EACCES;
 }
 
 static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
@@ -1193,6 +1281,8 @@ 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_truncate, hook_file_truncate),
 
 	LSM_HOOK_INIT(file_open, hook_file_open),
 };
diff --git a/security/landlock/fs.h b/security/landlock/fs.h
index 8db7acf9109b..488e4813680a 100644
--- a/security/landlock/fs.h
+++ b/security/landlock/fs.h
@@ -36,6 +36,24 @@ struct landlock_inode_security {
 	struct landlock_object __rcu *object;
 };
 
+/**
+ * struct landlock_file_security - File security blob
+ *
+ * This information is populated when opening a file in hook_file_open, and
+ * tracks the relevant Landlock access rights that were available at the time
+ * of opening the file. Other LSM hooks use these rights in order to authorize
+ * operations on already opened files.
+ */
+struct landlock_file_security {
+	/**
+	 * @allowed_access: Access rights that were available at the time of
+	 * opening the file. This is not necessarily the full set of access
+	 * rights available at that time, but it's the necessary subset as
+	 * needed to authorize later operations on the open file.
+	 */
+	access_mask_t allowed_access;
+};
+
 /**
  * struct landlock_superblock_security - Superblock security blob
  *
@@ -50,6 +68,12 @@ struct landlock_superblock_security {
 	atomic_long_t inode_refs;
 };
 
+static inline struct landlock_file_security *
+landlock_file(const struct file *const file)
+{
+	return file->f_security + landlock_blob_sizes.lbs_file;
+}
+
 static inline struct landlock_inode_security *
 landlock_inode(const struct inode *const inode)
 {
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/setup.c b/security/landlock/setup.c
index f8e8e980454c..3f196d2ce4f9 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -19,6 +19,7 @@ bool landlock_initialized __lsm_ro_after_init = false;
 
 struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
 	.lbs_cred = sizeof(struct landlock_cred_security),
+	.lbs_file = sizeof(struct landlock_file_security),
 	.lbs_inode = sizeof(struct landlock_inode_security),
 	.lbs_superblock = sizeof(struct landlock_superblock_security),
 };
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 45de42a027c5..87b28d14a1aa 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -406,9 +406,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 | \
@@ -422,7 +423,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.3

[PATCH v8 5/9] 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:

* 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.
* ftruncate() requires that the file was opened by a thread that had
  the truncate right for the file at the time of open(). (The rights
  are carried along with the opened file.)

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 87b28d14a1aa..718543fd3dfc 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);
@@ -3158,6 +3162,289 @@ TEST_F_FORK(layout1, proc_pipe)
 	ASSERT_EQ(0, close(pipe_fds[1]));
 }
 
+/* Invokes truncate(2) and returns its 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.
+ */
+static int test_creat(const char *const path)
+{
+	int fd = creat(path, 0600);
+
+	if (fd < 0)
+		return errno;
+
+	/*
+	 * Mixing error codes from close(2) and creat(2) should not lead to any
+	 * (access type) confusion for this test.
+	 */
+	if (close(fd) < 0)
+		return errno;
+	return 0;
+}
+
+/*
+ * 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;
+	int ruleset_fd;
+
+	/* Enable Landlock. */
+	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: truncate and open with O_TRUNC work, unless the
+	 * file is attempted to be opened for writing.
+	 */
+	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));
+	EXPECT_EQ(EACCES, test_creat(file_r));
+
+	/*
+	 * Checks write right: truncate and open with O_TRUNC work, unless the
+	 * file is attempted to be opened for reading.
+	 */
+	EXPECT_EQ(0, test_truncate(file_w));
+	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(0, test_creat(file_w));
+
+	/*
+	 * Checks "no rights" case: truncate works but all open attempts fail,
+	 * including creat.
+	 */
+	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_creat(file_none));
+}
+
+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;
+	int ruleset_fd;
+
+	/* Enable Landlock. */
+	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. */
+	EXPECT_EQ(0, test_truncate(file_rwt));
+	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
+
+	/* Checks read and write rights: no truncate variant works. */
+	EXPECT_EQ(EACCES, test_truncate(file_rw));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
+
+	/*
+	 * Checks read and truncate rights: truncation works.
+	 *
+	 * Note: Files can get truncated using open() even with O_RDONLY.
+	 */
+	EXPECT_EQ(0, test_truncate(file_rt));
+	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY | O_TRUNC));
+
+	/* Checks truncate right: truncate works, but can't open file. */
+	EXPECT_EQ(0, test_truncate(file_t));
+	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
+
+	/* Checks "no rights" case: No form of truncation works. */
+	EXPECT_EQ(EACCES, 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));
+
+	/*
+	 * Checks truncate right on directory: truncate works on contained
+	 * files.
+	 */
+	EXPECT_EQ(0, test_truncate(file_in_dir_t));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
+
+	/*
+	 * 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(file_in_dir_w));
+
+	ASSERT_EQ(0, unlink(file_in_dir_w));
+	EXPECT_EQ(0, test_creat(file_in_dir_w));
+}
+
+/* Invokes ftruncate(2) and returns its errno or 0. */
+static int test_ftruncate(int fd)
+{
+	if (ftruncate(fd, 10) < 0)
+		return errno;
+	return 0;
+}
+
+TEST_F_FORK(layout1, ftruncate)
+{
+	/*
+	 * This test opens a new file descriptor at different stages of
+	 * Landlock restriction:
+	 *
+	 * without restriction:                    ftruncate works
+	 * something else but truncate restricted: ftruncate works
+	 * truncate restricted and permitted:      ftruncate works
+	 * truncate restricted and not permitted:  ftruncate fails
+	 *
+	 * Whether this works or not is expected to depend on the time when the
+	 * FD was opened, not to depend on the time when ftruncate() was
+	 * called.
+	 */
+	const char *const path = file1_s1d1;
+	const __u64 handled1 = LANDLOCK_ACCESS_FS_READ_FILE |
+			       LANDLOCK_ACCESS_FS_WRITE_FILE;
+	const struct rule layer1[] = {
+		{
+			.path = path,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{},
+	};
+	const __u64 handled2 = LANDLOCK_ACCESS_FS_TRUNCATE;
+	const struct rule layer2[] = {
+		{
+			.path = path,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{},
+	};
+	const __u64 handled3 = LANDLOCK_ACCESS_FS_TRUNCATE |
+			       LANDLOCK_ACCESS_FS_WRITE_FILE;
+	const struct rule layer3[] = {
+		{
+			.path = path,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{},
+	};
+	int fd_layer0, fd_layer1, fd_layer2, fd_layer3, ruleset_fd;
+
+	fd_layer0 = open(path, O_WRONLY);
+	EXPECT_EQ(0, test_ftruncate(fd_layer0));
+
+	ruleset_fd = create_ruleset(_metadata, handled1, layer1);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	fd_layer1 = open(path, O_WRONLY);
+	EXPECT_EQ(0, test_ftruncate(fd_layer0));
+	EXPECT_EQ(0, test_ftruncate(fd_layer1));
+
+	ruleset_fd = create_ruleset(_metadata, handled2, layer2);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	fd_layer2 = open(path, O_WRONLY);
+	EXPECT_EQ(0, test_ftruncate(fd_layer0));
+	EXPECT_EQ(0, test_ftruncate(fd_layer1));
+	EXPECT_EQ(0, test_ftruncate(fd_layer2));
+
+	ruleset_fd = create_ruleset(_metadata, handled3, layer3);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	fd_layer3 = open(path, O_WRONLY);
+	EXPECT_EQ(0, test_ftruncate(fd_layer0));
+	EXPECT_EQ(0, test_ftruncate(fd_layer1));
+	EXPECT_EQ(0, test_ftruncate(fd_layer2));
+	EXPECT_EQ(EACCES, test_ftruncate(fd_layer3));
+
+	ASSERT_EQ(0, close(fd_layer0));
+	ASSERT_EQ(0, close(fd_layer1));
+	ASSERT_EQ(0, close(fd_layer2));
+	ASSERT_EQ(0, close(fd_layer3));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.3

[PATCH v8 6/9] selftests/landlock: Test open() and ftruncate() in multiple scenarios

[Günther Noack]

This test uses multiple fixture variants to exercise a broader set of
scnenarios.

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 718543fd3dfc..308f6f36e8c0 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3445,6 +3445,102 @@ TEST_F_FORK(layout1, ftruncate)
 	ASSERT_EQ(0, close(fd_layer3));
 }
 
+/* clang-format off */
+FIXTURE(ftruncate) {};
+/* clang-format on */
+
+FIXTURE_SETUP(ftruncate)
+{
+	prepare_layout(_metadata);
+	create_file(_metadata, file1_s1d1);
+}
+
+FIXTURE_TEARDOWN(ftruncate)
+{
+	EXPECT_EQ(0, remove_path(file1_s1d1));
+	cleanup_layout(_metadata);
+}
+
+FIXTURE_VARIANT(ftruncate)
+{
+	const __u64 handled;
+	const __u64 permitted;
+	const int expected_open_result;
+	const int expected_ftruncate_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ftruncate, w_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.expected_open_result = 0,
+	.expected_ftruncate_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ftruncate, t_t) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_TRUNCATE,
+	.permitted = LANDLOCK_ACCESS_FS_TRUNCATE,
+	.expected_open_result = 0,
+	.expected_ftruncate_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ftruncate, wt_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
+	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.expected_open_result = 0,
+	.expected_ftruncate_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ftruncate, wt_wt) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
+	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE |
+		     LANDLOCK_ACCESS_FS_TRUNCATE,
+	.expected_open_result = 0,
+	.expected_ftruncate_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ftruncate, wt_t) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
+	.permitted = LANDLOCK_ACCESS_FS_TRUNCATE,
+	.expected_open_result = EACCES,
+};
+
+TEST_F_FORK(ftruncate, open_and_ftruncate)
+{
+	const char *const path = file1_s1d1;
+	const struct rule rules[] = {
+		{
+			.path = path,
+			.access = variant->permitted,
+		},
+		{},
+	};
+	int fd, ruleset_fd;
+
+	/* Enable Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	fd = open(path, O_WRONLY);
+	EXPECT_EQ(variant->expected_open_result, (fd < 0 ? errno : 0));
+	if (fd >= 0) {
+		EXPECT_EQ(variant->expected_ftruncate_result,
+			  test_ftruncate(fd));
+		ASSERT_EQ(0, close(fd));
+	}
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.3

[PATCH v8 7/9] selftests/landlock: Test FD passing from a Landlock-restricted to an unrestricted process

[Günther Noack]

A file descriptor created in a restricted process carries Landlock
restrictions with it which will apply even if the same opened file is
used from an unrestricted process.

This change extracts suitable FD-passing helpers from base_test.c and
moves them to common.h. We use the fixture variants from the ftruncate
fixture to exercise the same scenarios as in the open_and_ftruncate
test, but doing the Landlock restriction and open() in a different
process than the ftruncate() call.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 tools/testing/selftests/landlock/base_test.c | 36 +----------
 tools/testing/selftests/landlock/common.h    | 67 ++++++++++++++++++++
 tools/testing/selftests/landlock/fs_test.c   | 62 ++++++++++++++++++
 3 files changed, 132 insertions(+), 33 deletions(-)

diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 72cdae277b02..6d1b6eedb432 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -263,23 +263,6 @@ TEST(ruleset_fd_transfer)
 		.allowed_access = LANDLOCK_ACCESS_FS_READ_DIR,
 	};
 	int ruleset_fd_tx, dir_fd;
-	union {
-		/* Aligned ancillary data buffer. */
-		char buf[CMSG_SPACE(sizeof(ruleset_fd_tx))];
-		struct cmsghdr _align;
-	} cmsg_tx = {};
-	char data_tx = '.';
-	struct iovec io = {
-		.iov_base = &data_tx,
-		.iov_len = sizeof(data_tx),
-	};
-	struct msghdr msg = {
-		.msg_iov = &io,
-		.msg_iovlen = 1,
-		.msg_control = &cmsg_tx.buf,
-		.msg_controllen = sizeof(cmsg_tx.buf),
-	};
-	struct cmsghdr *cmsg;
 	int socket_fds[2];
 	pid_t child;
 	int status;
@@ -298,33 +281,20 @@ TEST(ruleset_fd_transfer)
 				    &path_beneath_attr, 0));
 	ASSERT_EQ(0, close(path_beneath_attr.parent_fd));
 
-	cmsg = CMSG_FIRSTHDR(&msg);
-	ASSERT_NE(NULL, cmsg);
-	cmsg->cmsg_len = CMSG_LEN(sizeof(ruleset_fd_tx));
-	cmsg->cmsg_level = SOL_SOCKET;
-	cmsg->cmsg_type = SCM_RIGHTS;
-	memcpy(CMSG_DATA(cmsg), &ruleset_fd_tx, sizeof(ruleset_fd_tx));
-
 	/* Sends the ruleset FD over a socketpair and then close it. */
 	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
 				socket_fds));
-	ASSERT_EQ(sizeof(data_tx), sendmsg(socket_fds[0], &msg, 0));
+	ASSERT_EQ(0, send_fd(socket_fds[0], ruleset_fd_tx));
 	ASSERT_EQ(0, close(socket_fds[0]));
 	ASSERT_EQ(0, close(ruleset_fd_tx));
 
 	child = fork();
 	ASSERT_LE(0, child);
 	if (child == 0) {
-		int ruleset_fd_rx;
+		int ruleset_fd_rx = recv_fd(socket_fds[1]);
 
-		*(char *)msg.msg_iov->iov_base = '\0';
-		ASSERT_EQ(sizeof(data_tx),
-			  recvmsg(socket_fds[1], &msg, MSG_CMSG_CLOEXEC));
-		ASSERT_EQ('.', *(char *)msg.msg_iov->iov_base);
+		ASSERT_LE(0, ruleset_fd_rx);
 		ASSERT_EQ(0, close(socket_fds[1]));
-		cmsg = CMSG_FIRSTHDR(&msg);
-		ASSERT_EQ(cmsg->cmsg_len, CMSG_LEN(sizeof(ruleset_fd_tx)));
-		memcpy(&ruleset_fd_rx, CMSG_DATA(cmsg), sizeof(ruleset_fd_tx));
 
 		/* Enforces the received ruleset on the child. */
 		ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 7d34592471db..15149d34e43b 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -10,6 +10,7 @@
 #include <errno.h>
 #include <linux/landlock.h>
 #include <sys/capability.h>
+#include <sys/socket.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
 #include <sys/wait.h>
@@ -189,3 +190,69 @@ static void __maybe_unused clear_cap(struct __test_metadata *const _metadata,
 {
 	_effective_cap(_metadata, caps, CAP_CLEAR);
 }
+
+/* Receives an FD from a UNIX socket. Returns the received FD, -1 on error. */
+static int __maybe_unused recv_fd(int usock)
+{
+	int fd_rx;
+	union {
+		/* Aligned ancillary data buffer. */
+		char buf[CMSG_SPACE(sizeof(fd_rx))];
+		struct cmsghdr _align;
+	} cmsg_rx = {};
+	char data = '\0';
+	struct iovec io = {
+		.iov_base = &data,
+		.iov_len = sizeof(data),
+	};
+	struct msghdr msg = {
+		.msg_iov = &io,
+		.msg_iovlen = 1,
+		.msg_control = &cmsg_rx.buf,
+		.msg_controllen = sizeof(cmsg_rx.buf),
+	};
+	struct cmsghdr *cmsg;
+	int res;
+
+	res = recvmsg(usock, &msg, MSG_CMSG_CLOEXEC);
+	if (res < 0)
+		return -1;
+
+	cmsg = CMSG_FIRSTHDR(&msg);
+	if (cmsg->cmsg_len != CMSG_LEN(sizeof(fd_rx)))
+		return -1;
+
+	memcpy(&fd_rx, CMSG_DATA(cmsg), sizeof(fd_rx));
+	return fd_rx;
+}
+
+/* Sends an FD on a UNIX socket. Returns 0 on success or -1 on error. */
+static int __maybe_unused send_fd(int usock, int fd_tx)
+{
+	union {
+		/* Aligned ancillary data buffer. */
+		char buf[CMSG_SPACE(sizeof(fd_tx))];
+		struct cmsghdr _align;
+	} cmsg_tx = {};
+	char data_tx = '.';
+	struct iovec io = {
+		.iov_base = &data_tx,
+		.iov_len = sizeof(data_tx),
+	};
+	struct msghdr msg = {
+		.msg_iov = &io,
+		.msg_iovlen = 1,
+		.msg_control = &cmsg_tx.buf,
+		.msg_controllen = sizeof(cmsg_tx.buf),
+	};
+	struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
+
+	cmsg->cmsg_len = CMSG_LEN(sizeof(fd_tx));
+	cmsg->cmsg_level = SOL_SOCKET;
+	cmsg->cmsg_type = SCM_RIGHTS;
+	memcpy(CMSG_DATA(cmsg), &fd_tx, sizeof(fd_tx));
+
+	if (sendmsg(usock, &msg, 0) < 0)
+		return -1;
+	return 0;
+}
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 308f6f36e8c0..93ed80a25a74 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3541,6 +3541,68 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
 	}
 }
 
+TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
+{
+	int child, fd, status;
+	int socket_fds[2];
+
+	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
+				socket_fds));
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		/*
+		 * Enable Landlock in the child process, open a file descriptor
+		 * where truncation is forbidden and send it to the
+		 * non-landlocked parent process.
+		 */
+		const char *const path = file1_s1d1;
+		const struct rule rules[] = {
+			{
+				.path = path,
+				.access = variant->permitted,
+			},
+			{},
+		};
+		int fd, ruleset_fd;
+
+		/* Enable Landlock. */
+		ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+		ASSERT_LE(0, ruleset_fd);
+		enforce_ruleset(_metadata, ruleset_fd);
+		ASSERT_EQ(0, close(ruleset_fd));
+
+		fd = open(path, O_WRONLY);
+		ASSERT_EQ(variant->expected_open_result, (fd < 0 ? errno : 0));
+
+		if (fd >= 0) {
+			ASSERT_EQ(0, send_fd(socket_fds[0], fd));
+			ASSERT_EQ(0, close(fd));
+		}
+
+		ASSERT_EQ(0, close(socket_fds[0]));
+
+		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+	}
+
+	if (variant->expected_open_result == 0) {
+		fd = recv_fd(socket_fds[1]);
+		ASSERT_LE(0, fd);
+
+		EXPECT_EQ(variant->expected_ftruncate_result,
+			  test_ftruncate(fd));
+		ASSERT_EQ(0, close(fd));
+	}
+
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+	ASSERT_EQ(1, WIFEXITED(status));
+	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+	ASSERT_EQ(0, close(socket_fds[0]));
+	ASSERT_EQ(0, close(socket_fds[1]));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.3

[PATCH v8 8/9] 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, except 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.3

[PATCH v8 9/9] 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 | 66 +++++++++++++++++++++---
 1 file changed, 59 insertions(+), 7 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index b8ea59493964..44d6f598b63d 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -8,7 +8,7 @@ Landlock: unprivileged access control
 =====================================
 
 :Author: Mickaël Salaün
-:Date: May 2022
+:Date: October 2022
 
 The goal of Landlock is to enable to restrict ambient rights (e.g. global
 filesystem access) for a set of processes.  Because Landlock is a stackable
@@ -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,27 @@ 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`` or ``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) {
+    if (abi < 0) {
+        perror("The running kernel does not enable to use Landlock");
+        return 0;  /* Degrade gracefully if Landlock is not handled. */
+    }
+    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;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -127,8 +139,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 +263,37 @@ 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 require the
+``LANDLOCK_ACCESS_FS_WRITE_FILE`` right.  Apart from the :manpage:`truncate(2)`
+system call, this can also be done through :manpage:`open(2)` with the flags
+``O_RDONLY | O_TRUNC``.
+
+When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
+right is associated with the newly created file descriptor and will be used for
+subsequent truncation attempts using :manpage:`ftruncate(2)`.  The behavior is
+similar to opening a file for reading or writing, where permissions are checked
+during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
+:manpage:`write(2)` calls.
+
+As a consequence, it is possible to have multiple open file descriptors for the
+same file, where one grants the right to truncate the file and the other does
+not.  It is also possible to pass such file descriptors between processes,
+keeping their Landlock properties, even when these processes do not have an
+enforced Landlock ruleset.
+
 Compatibility
 =============
 
@@ -397,6 +440,15 @@ Starting with the Landlock ABI version 2, it is now possible to securely
 control renaming and linking thanks to the new `LANDLOCK_ACCESS_FS_REFER`
 access right.
 
+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.
+
 .. _kernel_support:
 
 Kernel support
-- 
2.37.3

Re: [PATCH v8 4/9] landlock: Support file truncation

[Nathan Chancellor]

Hi Günther,

On Sat, Oct 01, 2022 at 05:49:03PM +0200, 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 updates code documentation to document
> the flag.
> 
> The following operations are restricted:
> 
> open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
> implicitly truncated as part of the open() (e.g. using O_TRUNC).
> 
> Notable special cases:
> * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
> * open() with O_TRUNC does *not* need the TRUNCATE right when it
>   creates a new file.
> 
> truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
> right.
> 
> ftruncate() (on a file): requires that the file had the TRUNCATE right
> when it was previously opened.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>

I just bisected a crash in QEMU with Debian's arm64 configuration to
this change in -next as commit b40deebe7679 ("landlock: Support file
truncation"), which I was able to reproduce like so:

$ mkdir -p build/deb

$ cd build/deb

$ curl -LSsO http://ftp.us.debian.org/debian/pool/main/l/linux-signed-arm64/linux-image-5.19.0-2-arm64_5.19.11-1_arm64.deb

$ ar x linux-image-5.19.0-2-arm64_5.19.11-1_arm64.deb

$ tar xJf data.tar.xz

$ cp boot/config-5.19.0-2-arm64 ../.config

$ cd ../..

$ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- O=build olddefconfig Image.gz

$ qemu-system-aarch64 \
-machine virt,gic-version=max,virtualization=true \
-cpu max,pauth-impdef=true \
-kernel build/arch/arm64/boot/Image.gz \
-append "console=ttyAMA0 earlycon" \
-display none \
-initrd .../rootfs.cpio \
-m 512m \
-nodefaults \
-no-reboot \
-serial mon:stdio
...
[    0.000000] Linux version 6.0.0-rc7+ (nathan@dev-arch.thelio-3990X) (aarch64-linux-gnu-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #1 SMP Tue Oct 4 12:48:50 MST 2022
...
[    0.518570] Unable to handle kernel paging request at virtual address ffff00000851ff8a
[    0.518785] Mem abort info:
[    0.518867]   ESR = 0x0000000097c0c061
[    0.519001]   EC = 0x25: DABT (current EL), IL = 32 bits
[    0.519155]   SET = 0, FnV = 0
[    0.519267]   EA = 0, S1PTW = 0
[    0.519386]   FSC = 0x21: alignment fault
[    0.519524] Data abort info:
[    0.519615]   Access size = 8 byte(s)
[    0.519722]   SSE = 0, SRT = 0
[    0.519817]   SF = 1, AR = 1
[    0.519920]   CM = 0, WnR = 1
[    0.520040] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000041711000
[    0.520225] [ffff00000851ff8a] pgd=180000005fff8003, p4d=180000005fff8003, pud=180000005fff7003, pmd=180000005ffbd003, pte=006800004851ff07
[    0.521121] Internal error: Oops: 97c0c061 [#1] SMP
[    0.521364] Modules linked in:
[    0.521592] CPU: 0 PID: 9 Comm: kworker/u2:0 Not tainted 6.0.0-rc7+ #1
[    0.521863] Hardware name: linux,dummy-virt (DT)
[    0.522325] Workqueue: events_unbound async_run_entry_fn
[    0.522973] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.523193] pc : apparmor_file_alloc_security+0x98/0x1e0
[    0.523431] lr : apparmor_file_alloc_security+0x48/0x1e0
[    0.523594] sp : ffff800008093960
[    0.523708] x29: ffff800008093960 x28: ffff800008093b30 x27: ffff000002602600
[    0.523978] x26: ffffd79796ecf8c0 x25: ffff00000241e705 x24: ffffd79797d98068
[    0.524199] x23: ffff00000851ff82 x22: ffff00000851ff80 x21: 0000000000000002
[    0.524431] x20: ffffd79796ff5000 x19: ffff00000241ceb0 x18: ffffffffffffffff
[    0.524647] x17: 000000000000003f x16: ffffd79797678008 x15: 0000000000000000
[    0.524850] x14: 0000000000000001 x13: 0000000000000000 x12: 0000000000000006
[    0.525087] x11: ffff00001feef940 x10: ffffd7979768f8a0 x9 : ffffd79796c1e51c
[    0.525325] x8 : ffff00000851ffa0 x7 : 0000000000000000 x6 : 0000000000001e0b
[    0.525531] x5 : ffff00000851ff80 x4 : ffff800008093990 x3 : ffff000002419700
[    0.525745] x2 : 0000000000000001 x1 : ffff00000851ff8a x0 : ffff00000241ceb0
[    0.526034] Call trace:
[    0.526166]  apparmor_file_alloc_security+0x98/0x1e0
[    0.526424]  security_file_alloc+0x6c/0xf0
[    0.526570]  __alloc_file+0x5c/0xf0
[    0.526699]  alloc_empty_file+0x68/0x10c
[    0.526816]  path_openat+0x50/0x106c
[    0.526929]  do_filp_open+0x88/0x13c
[    0.527041]  filp_open+0x110/0x1b0
[    0.527143]  do_name+0xbc/0x230
[    0.527256]  write_buffer+0x40/0x60
[    0.527359]  unpack_to_rootfs+0x100/0x2bc
[    0.527479]  do_populate_rootfs+0x70/0x134
[    0.527602]  async_run_entry_fn+0x40/0x1c0
[    0.527723]  process_one_work+0x1f4/0x450
[    0.527851]  worker_thread+0x188/0x4c0
[    0.527980]  kthread+0xe0/0xe4
[    0.528066]  ret_from_fork+0x10/0x20
[    0.528317] Code: 52800002 d2800000 d2800013 910022e1 (c89ffc20)
[    0.528736] ---[ end trace 0000000000000000 ]---
...

A rootfs is available at [1] but I don't think it should be necessary
for reproducing this. If there is any additional information I can
provide or patches I can test, I am more than happy to do so!

[1]: https://github.com/ClangBuiltLinux/boot-utils/raw/bf2fd3500d87f78a914bfc3769b2240f5632e5b9/images/arm64/rootfs.cpio.zst

Cheers,
Nathan

Re: [PATCH v8 4/9] landlock: Support file truncation

[Mickaël Salaün]

On 04/10/2022 21:56, Nathan Chancellor wrote:
> Hi Günther,
> 
> On Sat, Oct 01, 2022 at 05:49:03PM +0200, 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 updates code documentation to document
>> the flag.
>>
>> The following operations are restricted:
>>
>> open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
>> implicitly truncated as part of the open() (e.g. using O_TRUNC).
>>
>> Notable special cases:
>> * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
>> * open() with O_TRUNC does *not* need the TRUNCATE right when it
>>    creates a new file.
>>
>> truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
>> right.
>>
>> ftruncate() (on a file): requires that the file had the TRUNCATE right
>> when it was previously opened.
>>
>> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> 
> I just bisected a crash in QEMU with Debian's arm64 configuration to
> this change in -next as commit b40deebe7679 ("landlock: Support file
> truncation"), which I was able to reproduce like so:

Thanks for the report Nathan. I've found an issue in this patch and 
fixed it in -next with this (rebased) commit: 
https://git.kernel.org/mic/c/b40deebe7679b05d4852488ef531e189a9621f2e
You should already have this update since I pushed it Monday.
Please let us know if this fixed the issue.


> 
> $ mkdir -p build/deb
> 
> $ cd build/deb
> 
> $ curl -LSsO http://ftp.us.debian.org/debian/pool/main/l/linux-signed-arm64/linux-image-5.19.0-2-arm64_5.19.11-1_arm64.deb
> 
> $ ar x linux-image-5.19.0-2-arm64_5.19.11-1_arm64.deb
> 
> $ tar xJf data.tar.xz
> 
> $ cp boot/config-5.19.0-2-arm64 ../.config
> 
> $ cd ../..
> 
> $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- O=build olddefconfig Image.gz
> 
> $ qemu-system-aarch64 \
> -machine virt,gic-version=max,virtualization=true \
> -cpu max,pauth-impdef=true \
> -kernel build/arch/arm64/boot/Image.gz \
> -append "console=ttyAMA0 earlycon" \
> -display none \
> -initrd .../rootfs.cpio \
> -m 512m \
> -nodefaults \
> -no-reboot \
> -serial mon:stdio
> ...
> [    0.000000] Linux version 6.0.0-rc7+ (nathan@dev-arch.thelio-3990X) (aarch64-linux-gnu-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #1 SMP Tue Oct 4 12:48:50 MST 2022
> ...
> [    0.518570] Unable to handle kernel paging request at virtual address ffff00000851ff8a
> [    0.518785] Mem abort info:
> [    0.518867]   ESR = 0x0000000097c0c061
> [    0.519001]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    0.519155]   SET = 0, FnV = 0
> [    0.519267]   EA = 0, S1PTW = 0
> [    0.519386]   FSC = 0x21: alignment fault
> [    0.519524] Data abort info:
> [    0.519615]   Access size = 8 byte(s)
> [    0.519722]   SSE = 0, SRT = 0
> [    0.519817]   SF = 1, AR = 1
> [    0.519920]   CM = 0, WnR = 1
> [    0.520040] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000041711000
> [    0.520225] [ffff00000851ff8a] pgd=180000005fff8003, p4d=180000005fff8003, pud=180000005fff7003, pmd=180000005ffbd003, pte=006800004851ff07
> [    0.521121] Internal error: Oops: 97c0c061 [#1] SMP
> [    0.521364] Modules linked in:
> [    0.521592] CPU: 0 PID: 9 Comm: kworker/u2:0 Not tainted 6.0.0-rc7+ #1
> [    0.521863] Hardware name: linux,dummy-virt (DT)
> [    0.522325] Workqueue: events_unbound async_run_entry_fn
> [    0.522973] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    0.523193] pc : apparmor_file_alloc_security+0x98/0x1e0
> [    0.523431] lr : apparmor_file_alloc_security+0x48/0x1e0
> [    0.523594] sp : ffff800008093960
> [    0.523708] x29: ffff800008093960 x28: ffff800008093b30 x27: ffff000002602600
> [    0.523978] x26: ffffd79796ecf8c0 x25: ffff00000241e705 x24: ffffd79797d98068
> [    0.524199] x23: ffff00000851ff82 x22: ffff00000851ff80 x21: 0000000000000002
> [    0.524431] x20: ffffd79796ff5000 x19: ffff00000241ceb0 x18: ffffffffffffffff
> [    0.524647] x17: 000000000000003f x16: ffffd79797678008 x15: 0000000000000000
> [    0.524850] x14: 0000000000000001 x13: 0000000000000000 x12: 0000000000000006
> [    0.525087] x11: ffff00001feef940 x10: ffffd7979768f8a0 x9 : ffffd79796c1e51c
> [    0.525325] x8 : ffff00000851ffa0 x7 : 0000000000000000 x6 : 0000000000001e0b
> [    0.525531] x5 : ffff00000851ff80 x4 : ffff800008093990 x3 : ffff000002419700
> [    0.525745] x2 : 0000000000000001 x1 : ffff00000851ff8a x0 : ffff00000241ceb0
> [    0.526034] Call trace:
> [    0.526166]  apparmor_file_alloc_security+0x98/0x1e0
> [    0.526424]  security_file_alloc+0x6c/0xf0
> [    0.526570]  __alloc_file+0x5c/0xf0
> [    0.526699]  alloc_empty_file+0x68/0x10c
> [    0.526816]  path_openat+0x50/0x106c
> [    0.526929]  do_filp_open+0x88/0x13c
> [    0.527041]  filp_open+0x110/0x1b0
> [    0.527143]  do_name+0xbc/0x230
> [    0.527256]  write_buffer+0x40/0x60
> [    0.527359]  unpack_to_rootfs+0x100/0x2bc
> [    0.527479]  do_populate_rootfs+0x70/0x134
> [    0.527602]  async_run_entry_fn+0x40/0x1c0
> [    0.527723]  process_one_work+0x1f4/0x450
> [    0.527851]  worker_thread+0x188/0x4c0
> [    0.527980]  kthread+0xe0/0xe4
> [    0.528066]  ret_from_fork+0x10/0x20
> [    0.528317] Code: 52800002 d2800000 d2800013 910022e1 (c89ffc20)
> [    0.528736] ---[ end trace 0000000000000000 ]---
> ...
> 
> A rootfs is available at [1] but I don't think it should be necessary
> for reproducing this. If there is any additional information I can
> provide or patches I can test, I am more than happy to do so!
> 
> [1]: https://github.com/ClangBuiltLinux/boot-utils/raw/bf2fd3500d87f78a914bfc3769b2240f5632e5b9/images/arm64/rootfs.cpio.zst
> 
> Cheers,
> Nathan

Re: [PATCH v8 1/9] security: Create file_truncate hook from path_truncate hook

[Mickaël Salaün]

Thanks for the doc.

On 01/10/2022 17:49, Günther Noack wrote:
> Like path_truncate, the file_truncate hook also restricts file
> truncation, but is called in the cases where truncation is attempted
> on an already-opened file.
> 
> This is required in a subsequent commit to handle ftruncate()
> operations differently to truncate() operations.
> 
> Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   fs/namei.c                    |  2 +-
>   fs/open.c                     |  2 +-
>   include/linux/lsm_hook_defs.h |  1 +
>   include/linux/lsm_hooks.h     | 10 +++++++++-
>   include/linux/security.h      |  6 ++++++
>   security/apparmor/lsm.c       |  6 ++++++
>   security/security.c           |  5 +++++
>   security/tomoyo/tomoyo.c      | 13 +++++++++++++
>   8 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 53b4bc094db2..0e419bd30f8e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
>   	if (error)
>   		return error;
>   
> -	error = security_path_truncate(path);
> +	error = security_file_truncate(filp);
>   	if (!error) {
>   		error = do_truncate(mnt_userns, path->dentry, 0,
>   				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> diff --git a/fs/open.c b/fs/open.c
> index cf7e5c350a54..0fa861873245 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>   	if (IS_APPEND(file_inode(f.file)))
>   		goto out_putf;
>   	sb_start_write(inode->i_sb);
> -	error = security_path_truncate(&f.file->f_path);
> +	error = security_file_truncate(f.file);
>   	if (!error)
>   		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>   				    ATTR_MTIME | ATTR_CTIME, f.file);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 60fff133c0b1..dee35ab253ba 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
>   	 struct fown_struct *fown, int sig)
>   LSM_HOOK(int, 0, file_receive, struct file *file)
>   LSM_HOOK(int, 0, file_open, struct file *file)
> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>   LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>   	 unsigned long clone_flags)
>   LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 3aa6030302f5..4acc975f28d9 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -409,7 +409,9 @@
>    *	@attr is the iattr structure containing the new file attributes.
>    *	Return 0 if permission is granted.
>    * @path_truncate:
> - *	Check permission before truncating a file.
> + *	Check permission before truncating the file indicated by path.
> + *      Note that truncation permissions may also be checked based on
> + *      already opened files, using the @file_truncate hook.

The documentation comments (mostly) use tabs, not spaces.


>    *	@path contains the path structure for the file.
>    *	Return 0 if permission is granted.
>    * @inode_getattr:
> @@ -598,6 +600,12 @@
>    *	to receive an open file descriptor via socket IPC.
>    *	@file contains the file structure being received.
>    *	Return 0 if permission is granted.
> + * @file_truncate:
> + *	Check permission before truncating a file, i.e. using ftruncate.
> + *	Note that truncation permission may also be checked based on the path,
> + *      using the @path_truncate hook.

Same here.


> + *	@file contains the file structure for the file.
> + *	Return 0 if permission is granted.
>    * @file_open:
>    *	Save open-time permission checking state for later use upon
>    *	file_permission, and recheck access if anything has changed
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 7bd0c490703d..f80b23382dd9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>   				 struct fown_struct *fown, int sig);
>   int security_file_receive(struct file *file);
>   int security_file_open(struct file *file);
> +int security_file_truncate(struct file *file);
>   int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>   void security_task_free(struct task_struct *task);
>   int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
>   	return 0;
>   }
>   
> +static inline int security_file_truncate(struct file *file)
> +{
> +	return 0;
> +}
> +
>   static inline int security_task_alloc(struct task_struct *task,
>   				      unsigned long clone_flags)
>   {
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index e29cade7b662..98ecb7f221b8 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
>   	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
>   }
>   
> +static int apparmor_file_truncate(struct file *file)
> +{
> +	return apparmor_path_truncate(&file->f_path);
> +}
> +
>   static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
>   				 const char *old_name)
>   {
> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>   	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>   	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>   
>   	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>   	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
> diff --git a/security/security.c b/security/security.c
> index 4b95de24bc8d..d73e423005c3 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1650,6 +1650,11 @@ int security_file_open(struct file *file)
>   	return fsnotify_perm(file, MAY_OPEN);
>   }
>   
> +int security_file_truncate(struct file *file)
> +{
> +	return call_int_hook(file_truncate, 0, file);
> +}
> +
>   int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
>   {
>   	int rc = lsm_task_alloc(task);
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 71e82d855ebf..af04a7b7eb28 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
>   	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>   }
>   
> +/**
> + * tomoyo_file_truncate - Target for security_file_truncate().
> + *
> + * @file: Pointer to "struct file".
> + *
> + * Returns 0 on success, negative value otherwise.
> + */
> +static int tomoyo_file_truncate(struct file *file)
> +{
> +	return tomoyo_path_truncate(&file->f_path);
> +}
> +
>   /**
>    * tomoyo_path_unlink - Target for security_path_unlink().
>    *
> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>   	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>   	LSM_HOOK_INIT(file_open, tomoyo_file_open),
> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>   	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>   	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>   	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),

Re: [PATCH v8 2/9] selftests/landlock: Locally define __maybe_unused

[Mickaël Salaün]

Thanks for cleaning this. Can you please move this patch just before the 
test patches?


On 01/10/2022 17:49, Günther Noack wrote:
> The checkpatch tool started to flag __attribute__(__unused__), which
> we previously used. The header where this is normally defined is not
> currently compatible with selftests.
> 
> This is the same approach as used in selftests/net/psock_lib.h.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/common.h | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 7ba18eb23783..7d34592471db 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -17,6 +17,10 @@
>   
>   #include "../kselftest_harness.h"
>   
> +#ifndef __maybe_unused
> +#define __maybe_unused __attribute__((__unused__))
> +#endif
> +
>   /*
>    * TEST_F_FORK() is useful when a test drop privileges but the corresponding
>    * FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
> @@ -140,14 +144,12 @@ static void _init_caps(struct __test_metadata *const _metadata, bool drop_all)
>   }
>   
>   /* We cannot put such helpers in a library because of kselftest_harness.h . */
> -__attribute__((__unused__)) static void
> -disable_caps(struct __test_metadata *const _metadata)
> +static void __maybe_unused disable_caps(struct __test_metadata *const _metadata)
>   {
>   	_init_caps(_metadata, false);
>   }
>   
> -__attribute__((__unused__)) static void
> -drop_caps(struct __test_metadata *const _metadata)
> +static void __maybe_unused drop_caps(struct __test_metadata *const _metadata)
>   {
>   	_init_caps(_metadata, true);
>   }
> @@ -176,14 +178,14 @@ static void _effective_cap(struct __test_metadata *const _metadata,
>   	}
>   }
>   
> -__attribute__((__unused__)) static void
> -set_cap(struct __test_metadata *const _metadata, const cap_value_t caps)
> +static void __maybe_unused set_cap(struct __test_metadata *const _metadata,
> +				   const cap_value_t caps)
>   {
>   	_effective_cap(_metadata, caps, CAP_SET);
>   }
>   
> -__attribute__((__unused__)) static void
> -clear_cap(struct __test_metadata *const _metadata, const cap_value_t caps)
> +static void __maybe_unused clear_cap(struct __test_metadata *const _metadata,
> +				     const cap_value_t caps)
>   {
>   	_effective_cap(_metadata, caps, CAP_CLEAR);
>   }

Re: [PATCH v8 3/9] landlock: Refactor check_access_path_dual() into is_access_to_paths_allowed()

[Mickaël Salaün]

Great!

On 01/10/2022 17:49, Günther Noack wrote:
> * Rename it to is_access_to_paths_allowed()
> * Make it return true iff the access is allowed
> * Calculate the EXDEV/EACCES error code in the one place where it's needed

Can you please replace these bullet points with (one-sentence) paragraphs?


> 
> Suggested-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   security/landlock/fs.c | 89 +++++++++++++++++++++---------------------
>   1 file changed, 44 insertions(+), 45 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index a9dbd99d9ee7..083dd3d359de 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -430,7 +430,7 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
>   }
>   
>   /**
> - * check_access_path_dual - Check accesses for requests with a common path
> + * is_access_to_paths_allowed - Check accesses for requests with a common path
>    *
>    * @domain: Domain to check against.
>    * @path: File hierarchy to walk through.
> @@ -465,14 +465,10 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
>    * allow the request.
>    *
>    * Returns:
> - * - 0 if the access request is granted;
> - * - -EACCES if it is denied because of access right other than
> - *   LANDLOCK_ACCESS_FS_REFER;
> - * - -EXDEV if the renaming or linking would be a privileged escalation
> - *   (according to each layered policies), or if LANDLOCK_ACCESS_FS_REFER is
> - *   not allowed by the source or the destination.
> + * - true if the access request is granted;
> + * - false otherwise

Missing final dot.


>    */
> -static int check_access_path_dual(
> +static bool is_access_to_paths_allowed(
>   	const struct landlock_ruleset *const domain,
>   	const struct path *const path,
>   	const access_mask_t access_request_parent1,
> @@ -492,17 +488,17 @@ static int check_access_path_dual(
>   	(*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL;
>   
>   	if (!access_request_parent1 && !access_request_parent2)
> -		return 0;
> +		return true;
>   	if (WARN_ON_ONCE(!domain || !path))
> -		return 0;
> +		return true;
>   	if (is_nouser_or_private(path->dentry))
> -		return 0;
> +		return true;
>   	if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1))
> -		return -EACCES;
> +		return false;
>   
>   	if (unlikely(layer_masks_parent2)) {
>   		if (WARN_ON_ONCE(!dentry_child1))
> -			return -EACCES;
> +			return false;
>   		/*
>   		 * For a double request, first check for potential privilege
>   		 * escalation by looking at domain handled accesses (which are
> @@ -513,7 +509,7 @@ static int check_access_path_dual(
>   		is_dom_check = true;
>   	} else {
>   		if (WARN_ON_ONCE(dentry_child1 || dentry_child2))
> -			return -EACCES;
> +			return false;
>   		/* For a simple request, only check for requested accesses. */
>   		access_masked_parent1 = access_request_parent1;
>   		access_masked_parent2 = access_request_parent2;
> @@ -622,24 +618,7 @@ static int check_access_path_dual(
>   	}
>   	path_put(&walker_path);
>   
> -	if (allowed_parent1 && allowed_parent2)
> -		return 0;
> -
> -	/*
> -	 * This prioritizes EACCES over EXDEV for all actions, including
> -	 * renames with RENAME_EXCHANGE.
> -	 */
> -	if (likely(is_eacces(layer_masks_parent1, access_request_parent1) ||
> -		   is_eacces(layer_masks_parent2, access_request_parent2)))
> -		return -EACCES;
> -
> -	/*
> -	 * Gracefully forbids reparenting if the destination directory
> -	 * hierarchy is not a superset of restrictions of the source directory
> -	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
> -	 * source or the destination.
> -	 */
> -	return -EXDEV;
> +	return allowed_parent1 && allowed_parent2;
>   }
>   
>   static inline int check_access_path(const struct landlock_ruleset *const domain,
> @@ -649,8 +628,10 @@ static inline int check_access_path(const struct landlock_ruleset *const domain,
>   	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>   
>   	access_request = init_layer_masks(domain, access_request, &layer_masks);
> -	return check_access_path_dual(domain, path, access_request,
> -				      &layer_masks, NULL, 0, NULL, NULL);
> +	if (is_access_to_paths_allowed(domain, path, access_request,
> +				       &layer_masks, NULL, 0, NULL, NULL))
> +		return 0;
> +	return -EACCES;
>   }
>   
>   static inline int current_check_access_path(const struct path *const path,
> @@ -711,8 +692,9 @@ static inline access_mask_t maybe_remove(const struct dentry *const dentry)
>    * file.  While walking from @dir to @mnt_root, we record all the domain's
>    * allowed accesses in @layer_masks_dom.
>    *
> - * This is similar to check_access_path_dual() but much simpler because it only
> - * handles walking on the same mount point and only check one set of accesses.
> + * This is similar to is_access_to_paths_allowed() but much simpler because it
> + * only handles walking on the same mount point and only checks one set of
> + * accesses.
>    *
>    * Returns:
>    * - true if all the domain access rights are allowed for @dir;
> @@ -857,10 +839,11 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>   		access_request_parent1 = init_layer_masks(
>   			dom, access_request_parent1 | access_request_parent2,
>   			&layer_masks_parent1);
> -		return check_access_path_dual(dom, new_dir,
> -					      access_request_parent1,
> -					      &layer_masks_parent1, NULL, 0,
> -					      NULL, NULL);
> +		if (is_access_to_paths_allowed(
> +			    dom, new_dir, access_request_parent1,
> +			    &layer_masks_parent1, NULL, 0, NULL, NULL))
> +			return 0;
> +		return -EACCES;
>   	}
>   
>   	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
> @@ -886,11 +869,27 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>   	 * parent access rights.  This will be useful to compare with the
>   	 * destination parent access rights.
>   	 */
> -	return check_access_path_dual(dom, &mnt_dir, access_request_parent1,
> -				      &layer_masks_parent1, old_dentry,
> -				      access_request_parent2,
> -				      &layer_masks_parent2,
> -				      exchange ? new_dentry : NULL);
> +	if (is_access_to_paths_allowed(
> +		    dom, &mnt_dir, access_request_parent1, &layer_masks_parent1,
> +		    old_dentry, access_request_parent2, &layer_masks_parent2,
> +		    exchange ? new_dentry : NULL))
> +		return 0;
> +
> +	/*
> +	 * This prioritizes EACCES over EXDEV for all actions, including
> +	 * renames with RENAME_EXCHANGE.
> +	 */
> +	if (likely(is_eacces(&layer_masks_parent1, access_request_parent1) ||
> +		   is_eacces(&layer_masks_parent2, access_request_parent2)))
> +		return -EACCES;
> +
> +	/*
> +	 * Gracefully forbids reparenting if the destination directory
> +	 * hierarchy is not a superset of restrictions of the source directory
> +	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
> +	 * source or the destination.
> +	 */
> +	return -EXDEV;
>   }
>   
>   /* Inode hooks */

Re: [PATCH v8 4/9] landlock: Support file truncation

[Mickaël Salaün]

On 01/10/2022 17:49, 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 updates code documentation to document
> the flag.
> 
> The following operations are restricted:
> 
> open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
> implicitly truncated as part of the open() (e.g. using O_TRUNC).
> 
> Notable special cases:
> * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
> * open() with O_TRUNC does *not* need the TRUNCATE right when it
>    creates a new file.
> 
> truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
> right.
> 
> ftruncate() (on a file): requires that the file had the TRUNCATE right
> when it was previously opened.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   include/uapi/linux/landlock.h                |  21 +++-
>   security/landlock/fs.c                       | 102 +++++++++++++++++--
>   security/landlock/fs.h                       |  24 +++++
>   security/landlock/limits.h                   |   2 +-
>   security/landlock/setup.c                    |   1 +
>   security/landlock/syscalls.c                 |   2 +-
>   tools/testing/selftests/landlock/base_test.c |   2 +-
>   tools/testing/selftests/landlock/fs_test.c   |   7 +-
>   8 files changed, 144 insertions(+), 17 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..d830cdfdbe56 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -95,8 +95,19 @@ 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

%LANDLOCK_ACCESS_FS_TRUNCATE


> + *   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 with :manpage:`truncate(2)`,
> + *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
> + *   `O_TRUNC`. Whether an opened file can be truncated with

%O_TRUNC


> + *   :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
> + *   same way as read and write permissions are checked during
> + *   :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
> + *   %LANDLOCK_ACCESS_FS_WRITE_FILE. 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 +150,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 +170,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 083dd3d359de..80d507ce2305 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 */
>   
>   /*
> @@ -297,6 +298,18 @@ get_handled_accesses(const struct landlock_ruleset *const domain)
>   	return access_dom & LANDLOCK_MASK_ACCESS_FS;
>   }
>   
> +/*
> + * init_layer_masks - Populates @layer_masks such that for each access right in
> + * @access_request, the bits for all the layers are set where this access right
> + * is handled.

Thanks for this extra documentation!

Can you convert it to a proper code documentation (even if it not used 
yet), with a heading `/**` and a short title following the function 
name? Something like "init_layer_masks - Initialize layer masks". Please 
follow this convention for the other doc strings, or just use a 
paragraph in a simple comment (e.g. for get_required_file_open_access).

Because there is no direct link with Landlock supporting truncation, 
this should be in a standalone patch, but you can keep it in this series.


> + *
> + * @domain: The domain that defines the current restrictions.
> + * @access_request: The requested access rights to check.
> + * @layer_masks: The layer masks to populate.
> + *
> + * Returns: An access mask where each access right bit is set which is handled
> + * in any of the active layers in @domain.
> + */
>   static inline access_mask_t
>   init_layer_masks(const struct landlock_ruleset *const domain,
>   		 const access_mask_t access_request,
> @@ -1141,9 +1154,19 @@ 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)
> +/*
> + * get_required_file_open_access - Returns the access rights that are required
> + * for opening the file, depending on the file type and open mode.
> + */
> +static inline access_mask_t
> +get_required_file_open_access(const struct file *const file)
>   {
>   	access_mask_t access = 0;
>   
> @@ -1163,17 +1186,82 @@ static inline access_mask_t get_file_access(const struct file *const file)
>   
>   static int hook_file_open(struct file *const file)
>   {
> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> +	access_mask_t open_access_request, full_access_request, allowed_access;
> +	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
>   	const struct landlock_ruleset *const dom =
>   		landlock_get_current_domain();
>   
> -	if (!dom)
> +	if (!dom) {
> +		/*
> +		 * Grants all access rights, even if most of them are not
> +		 * checked later on. It is more consistent.
> +		 */
> +		landlock_file(file)->allowed_access = LANDLOCK_MASK_ACCESS_FS;

This looks like the right approach but unfortunately, because there is 
multiple ways to get a file descriptors (e.g. memfd_create, which is 
worth mentioning in a comment), this doesn't work well. For now, it only 
makes sense for Landlock to restrict file descriptors obtained through 
open(2). We can then move this initialization to a new hook 
implementation for file_alloc_security.

I think this is the bug Nathan reported.

We should have a test with memfd_create(2) to make sure it works as 
expected. I think the documentation is still correct though.



>   		return 0;
> +	}
> +
>   	/*
> -	 * Because a file may be opened with O_PATH, get_file_access() may
> -	 * return 0.  This case will be handled with a future Landlock
> +	 * Because a file may be opened with O_PATH, get_required_file_open_access()
> +	 * may return 0.  This case will be handled with a future Landlock
>   	 * evolution.
>   	 */
> -	return check_access_path(dom, &file->f_path, get_file_access(file));
> +	open_access_request = get_required_file_open_access(file);
> +
> +	/*
> +	 * We look up more access than what we immediately need for open(), so
> +	 * that we can later authorize operations on opened files.
> +	 */
> +	full_access_request = open_access_request | optional_access;
> +
> +	allowed_access = full_access_request;
> +	if (!is_access_to_paths_allowed(
> +		    dom, &file->f_path,
> +		    init_layer_masks(dom, full_access_request, &layer_masks),
> +		    &layer_masks, NULL, 0, NULL, NULL)) {

I'd prefer (less error prone and easier to read) to add an 
is_access_paths_allowed branch to initialize allowed_access with 
full_access_request, and tweak this branch to initialize allowed_access 
with 0 and then populate it according to !layer_masks[access_bit].


> +		unsigned long access_bit;
> +		unsigned long access_req = full_access_request;

const unsigned long access_req


> +
> +		/*
> +		 * Calculate the actual allowed access rights from layer_masks.
> +		 * Remove each access right from allowed_access which has been
> +		 * vetoed by any layer.
> +		 */
> +		for_each_set_bit(access_bit, &access_req,
> +				 ARRAY_SIZE(layer_masks)) {
> +			if (layer_masks[access_bit])
> +				allowed_access &= ~BIT_ULL(access_bit); > +		}
> +	}

We can move `landlock_file(file)->allowed_access = allowed_access` here 
to be sure that the struct file allowed access is consistent even if it 
should not be used (because access may be denied).


> +
> +	if (open_access_request & ~allowed_access)
> +		return -EACCES;

And here invert the check ((open_access_request & allowed_access) == 
open_access_request) to make it more consistent with other checks…


> +
> +	/*
> +	 * For operations on already opened files (i.e. ftruncate()), it is the
> +	 * access rights at the time of open() which decide whether the
> +	 * operation is permitted. Therefore, we record the relevant subset of
> +	 * file access rights in the opened struct file.
> +	 */
> +	landlock_file(file)->allowed_access = allowed_access;
> +	return 0;

…and return -EACCES here.


> +}
> +
> +static int hook_file_truncate(struct file *const file)
> +{
> +	/*
> +	 * Allows truncation if the truncate right was available at the time of
> +	 * opening the file, to get a consistent access check as for read, write
> +	 * and execute operations.
> +	 *
> +	 * Note: For checks done based on the file's Landlock rights, we enforce

s/file's Landlock rights/file's Landlock allowed access/ maybe?


> +	 * them independently of whether the current thread is in a Landlock
> +	 * domain, so that open files passed between independent processes
> +	 * retain their behaviour.
> +	 */
> +	if (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_TRUNCATE)
> +		return 0;
> +	return -EACCES;
>   }
>   
>   static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> @@ -1193,6 +1281,8 @@ 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_truncate, hook_file_truncate),

Please move the hook_file_truncate entry after the hook_file_open one, 
these entries are in the same order as their hook implementations.


>   
>   	LSM_HOOK_INIT(file_open, hook_file_open),
>   };
> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> index 8db7acf9109b..488e4813680a 100644
> --- a/security/landlock/fs.h
> +++ b/security/landlock/fs.h
> @@ -36,6 +36,24 @@ struct landlock_inode_security {
>   	struct landlock_object __rcu *object;
>   };
>   
> +/**
> + * struct landlock_file_security - File security blob
> + *
> + * This information is populated when opening a file in hook_file_open, and
> + * tracks the relevant Landlock access rights that were available at the time
> + * of opening the file. Other LSM hooks use these rights in order to authorize
> + * operations on already opened files.
> + */
> +struct landlock_file_security {
> +	/**
> +	 * @allowed_access: Access rights that were available at the time of
> +	 * opening the file. This is not necessarily the full set of access
> +	 * rights available at that time, but it's the necessary subset as
> +	 * needed to authorize later operations on the open file.
> +	 */
> +	access_mask_t allowed_access;
> +};
> +
>   /**
>    * struct landlock_superblock_security - Superblock security blob
>    *
> @@ -50,6 +68,12 @@ struct landlock_superblock_security {
>   	atomic_long_t inode_refs;
>   };
>   
> +static inline struct landlock_file_security *
> +landlock_file(const struct file *const file)
> +{
> +	return file->f_security + landlock_blob_sizes.lbs_file;
> +}
> +
>   static inline struct landlock_inode_security *
>   landlock_inode(const struct inode *const inode)
>   {
> 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/setup.c b/security/landlock/setup.c
> index f8e8e980454c..3f196d2ce4f9 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -19,6 +19,7 @@ bool landlock_initialized __lsm_ro_after_init = false;
>   
>   struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
>   	.lbs_cred = sizeof(struct landlock_cred_security),
> +	.lbs_file = sizeof(struct landlock_file_security),
>   	.lbs_inode = sizeof(struct landlock_inode_security),
>   	.lbs_superblock = sizeof(struct landlock_superblock_security),
>   };
> 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 45de42a027c5..87b28d14a1aa 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -406,9 +406,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 | \
> @@ -422,7 +423,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 v8 6/9] selftests/landlock: Test open() and ftruncate() in multiple scenarios

[Mickaël Salaün]

Clean test cases and complete coverage, great!

On 01/10/2022 17:49, Günther Noack wrote:
> This test uses multiple fixture variants to exercise a broader set of
> scnenarios.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 96 ++++++++++++++++++++++
>   1 file changed, 96 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 718543fd3dfc..308f6f36e8c0 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3445,6 +3445,102 @@ TEST_F_FORK(layout1, ftruncate)
>   	ASSERT_EQ(0, close(fd_layer3));
>   }
>   
> +/* clang-format off */
> +FIXTURE(ftruncate) {};
> +/* clang-format on */
> +
> +FIXTURE_SETUP(ftruncate)
> +{
> +	prepare_layout(_metadata);
> +	create_file(_metadata, file1_s1d1);
> +}
> +
> +FIXTURE_TEARDOWN(ftruncate)
> +{
> +	EXPECT_EQ(0, remove_path(file1_s1d1));
> +	cleanup_layout(_metadata);
> +}
> +
> +FIXTURE_VARIANT(ftruncate)
> +{
> +	const __u64 handled;
> +	const __u64 permitted;
> +	const int expected_open_result;
> +	const int expected_ftruncate_result;
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(ftruncate, w_w) {
> +	/* clang-format on */
> +	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +	.expected_open_result = 0,
> +	.expected_ftruncate_result = 0,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(ftruncate, t_t) {
> +	/* clang-format on */
> +	.handled = LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.permitted = LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.expected_open_result = 0,
> +	.expected_ftruncate_result = 0,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(ftruncate, wt_w) {
> +	/* clang-format on */
> +	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +	.expected_open_result = 0,
> +	.expected_ftruncate_result = EACCES,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(ftruncate, wt_wt) {
> +	/* clang-format on */
> +	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE |
> +		     LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.expected_open_result = 0,
> +	.expected_ftruncate_result = 0,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(ftruncate, wt_t) {
> +	/* clang-format on */
> +	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.permitted = LANDLOCK_ACCESS_FS_TRUNCATE,
> +	.expected_open_result = EACCES,
> +};
> +
> +TEST_F_FORK(ftruncate, open_and_ftruncate)
> +{
> +	const char *const path = file1_s1d1;
> +	const struct rule rules[] = {
> +		{
> +			.path = path,
> +			.access = variant->permitted,
> +		},
> +		{},
> +	};
> +	int fd, ruleset_fd;
> +
> +	/* Enable Landlock. */
> +	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	fd = open(path, O_WRONLY);
> +	EXPECT_EQ(variant->expected_open_result, (fd < 0 ? errno : 0));
> +	if (fd >= 0) {
> +		EXPECT_EQ(variant->expected_ftruncate_result,
> +			  test_ftruncate(fd));
> +		ASSERT_EQ(0, close(fd));
> +	}
> +}
> +
>   /* clang-format off */
>   FIXTURE(layout1_bind) {};
>   /* clang-format on */

Re: [PATCH v8 7/9] selftests/landlock: Test FD passing from a Landlock-restricted to an unrestricted process

[Mickaël Salaün]

Thanks for this refactoring.

We need a shorter subject though. ;)


On 01/10/2022 17:49, Günther Noack wrote:
> A file descriptor created in a restricted process carries Landlock
> restrictions with it which will apply even if the same opened file is
> used from an unrestricted process.
> 
> This change extracts suitable FD-passing helpers from base_test.c and
> moves them to common.h. We use the fixture variants from the ftruncate
> fixture to exercise the same scenarios as in the open_and_ftruncate
> test, but doing the Landlock restriction and open() in a different
> process than the ftruncate() call.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/base_test.c | 36 +----------
>   tools/testing/selftests/landlock/common.h    | 67 ++++++++++++++++++++
>   tools/testing/selftests/landlock/fs_test.c   | 62 ++++++++++++++++++
>   3 files changed, 132 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 72cdae277b02..6d1b6eedb432 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -263,23 +263,6 @@ TEST(ruleset_fd_transfer)
>   		.allowed_access = LANDLOCK_ACCESS_FS_READ_DIR,
>   	};
>   	int ruleset_fd_tx, dir_fd;
> -	union {
> -		/* Aligned ancillary data buffer. */
> -		char buf[CMSG_SPACE(sizeof(ruleset_fd_tx))];
> -		struct cmsghdr _align;
> -	} cmsg_tx = {};
> -	char data_tx = '.';
> -	struct iovec io = {
> -		.iov_base = &data_tx,
> -		.iov_len = sizeof(data_tx),
> -	};
> -	struct msghdr msg = {
> -		.msg_iov = &io,
> -		.msg_iovlen = 1,
> -		.msg_control = &cmsg_tx.buf,
> -		.msg_controllen = sizeof(cmsg_tx.buf),
> -	};
> -	struct cmsghdr *cmsg;
>   	int socket_fds[2];
>   	pid_t child;
>   	int status;
> @@ -298,33 +281,20 @@ TEST(ruleset_fd_transfer)
>   				    &path_beneath_attr, 0));
>   	ASSERT_EQ(0, close(path_beneath_attr.parent_fd));
>   
> -	cmsg = CMSG_FIRSTHDR(&msg);
> -	ASSERT_NE(NULL, cmsg);
> -	cmsg->cmsg_len = CMSG_LEN(sizeof(ruleset_fd_tx));
> -	cmsg->cmsg_level = SOL_SOCKET;
> -	cmsg->cmsg_type = SCM_RIGHTS;
> -	memcpy(CMSG_DATA(cmsg), &ruleset_fd_tx, sizeof(ruleset_fd_tx));
> -
>   	/* Sends the ruleset FD over a socketpair and then close it. */
>   	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
>   				socket_fds));
> -	ASSERT_EQ(sizeof(data_tx), sendmsg(socket_fds[0], &msg, 0));
> +	ASSERT_EQ(0, send_fd(socket_fds[0], ruleset_fd_tx));
>   	ASSERT_EQ(0, close(socket_fds[0]));
>   	ASSERT_EQ(0, close(ruleset_fd_tx));
>   
>   	child = fork();
>   	ASSERT_LE(0, child);
>   	if (child == 0) {
> -		int ruleset_fd_rx;
> +		int ruleset_fd_rx = recv_fd(socket_fds[1]);

We can now make it const.


>   
> -		*(char *)msg.msg_iov->iov_base = '\0';
> -		ASSERT_EQ(sizeof(data_tx),
> -			  recvmsg(socket_fds[1], &msg, MSG_CMSG_CLOEXEC));
> -		ASSERT_EQ('.', *(char *)msg.msg_iov->iov_base);
> +		ASSERT_LE(0, ruleset_fd_rx);
>   		ASSERT_EQ(0, close(socket_fds[1]));
> -		cmsg = CMSG_FIRSTHDR(&msg);
> -		ASSERT_EQ(cmsg->cmsg_len, CMSG_LEN(sizeof(ruleset_fd_tx)));
> -		memcpy(&ruleset_fd_rx, CMSG_DATA(cmsg), sizeof(ruleset_fd_tx));
>   
>   		/* Enforces the received ruleset on the child. */
>   		ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 7d34592471db..15149d34e43b 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -10,6 +10,7 @@
>   #include <errno.h>
>   #include <linux/landlock.h>
>   #include <sys/capability.h>
> +#include <sys/socket.h>
>   #include <sys/syscall.h>
>   #include <sys/types.h>
>   #include <sys/wait.h>
> @@ -189,3 +190,69 @@ static void __maybe_unused clear_cap(struct __test_metadata *const _metadata,
>   {
>   	_effective_cap(_metadata, caps, CAP_CLEAR);
>   }
> +
> +/* Receives an FD from a UNIX socket. Returns the received FD, -1 on error. */
> +static int __maybe_unused recv_fd(int usock)
> +{
> +	int fd_rx;
> +	union {
> +		/* Aligned ancillary data buffer. */
> +		char buf[CMSG_SPACE(sizeof(fd_rx))];
> +		struct cmsghdr _align;
> +	} cmsg_rx = {};
> +	char data = '\0';
> +	struct iovec io = {
> +		.iov_base = &data,
> +		.iov_len = sizeof(data),
> +	};
> +	struct msghdr msg = {
> +		.msg_iov = &io,
> +		.msg_iovlen = 1,
> +		.msg_control = &cmsg_rx.buf,
> +		.msg_controllen = sizeof(cmsg_rx.buf),
> +	};
> +	struct cmsghdr *cmsg;
> +	int res;
> +
> +	res = recvmsg(usock, &msg, MSG_CMSG_CLOEXEC);
> +	if (res < 0)
> +		return -1;

It could be useful to return -errno for recv_fd() and send_fd(), and 
update the description accordingly. That would enable to quickly know 
the error with the ASSERT_*() calls.


> +
> +	cmsg = CMSG_FIRSTHDR(&msg);
> +	if (cmsg->cmsg_len != CMSG_LEN(sizeof(fd_rx)))
> +		return -1;
> +
> +	memcpy(&fd_rx, CMSG_DATA(cmsg), sizeof(fd_rx));
> +	return fd_rx;
> +}
> +
> +/* Sends an FD on a UNIX socket. Returns 0 on success or -1 on error. */
> +static int __maybe_unused send_fd(int usock, int fd_tx)
> +{
> +	union {
> +		/* Aligned ancillary data buffer. */
> +		char buf[CMSG_SPACE(sizeof(fd_tx))];
> +		struct cmsghdr _align;
> +	} cmsg_tx = {};
> +	char data_tx = '.';
> +	struct iovec io = {
> +		.iov_base = &data_tx,
> +		.iov_len = sizeof(data_tx),
> +	};
> +	struct msghdr msg = {
> +		.msg_iov = &io,
> +		.msg_iovlen = 1,
> +		.msg_control = &cmsg_tx.buf,
> +		.msg_controllen = sizeof(cmsg_tx.buf),
> +	};
> +	struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
> +
> +	cmsg->cmsg_len = CMSG_LEN(sizeof(fd_tx));
> +	cmsg->cmsg_level = SOL_SOCKET;
> +	cmsg->cmsg_type = SCM_RIGHTS;
> +	memcpy(CMSG_DATA(cmsg), &fd_tx, sizeof(fd_tx));
> +
> +	if (sendmsg(usock, &msg, 0) < 0)
> +		return -1;
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 308f6f36e8c0..93ed80a25a74 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3541,6 +3541,68 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
>   	}
>   }
>   
> +TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
> +{
> +	int child, fd, status;
> +	int socket_fds[2];
> +
> +	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
> +				socket_fds));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		/*
> +		 * Enable Landlock in the child process, open a file descriptor

"Enables"…


> +		 * where truncation is forbidden and send it to the
> +		 * non-landlocked parent process.
> +		 */
> +		const char *const path = file1_s1d1;
> +		const struct rule rules[] = {
> +			{
> +				.path = path,
> +				.access = variant->permitted,
> +			},
> +			{},
> +		};
> +		int fd, ruleset_fd;
> +
> +		/* Enable Landlock. */

This comment is not necessary.


> +		ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> +		ASSERT_LE(0, ruleset_fd);
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		ASSERT_EQ(0, close(ruleset_fd));
> +
> +		fd = open(path, O_WRONLY);
> +		ASSERT_EQ(variant->expected_open_result, (fd < 0 ? errno : 0));
> +
> +		if (fd >= 0) {
> +			ASSERT_EQ(0, send_fd(socket_fds[0], fd));
> +			ASSERT_EQ(0, close(fd));
> +		}
> +
> +		ASSERT_EQ(0, close(socket_fds[0]));
> +
> +		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);

You might want to add a "return" here to help the compiler (and the reader).


> +	}
> +
> +	if (variant->expected_open_result == 0) {
> +		fd = recv_fd(socket_fds[1]);
> +		ASSERT_LE(0, fd);
> +
> +		EXPECT_EQ(variant->expected_ftruncate_result,
> +			  test_ftruncate(fd));
> +		ASSERT_EQ(0, close(fd));
> +	}
> +
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +	ASSERT_EQ(1, WIFEXITED(status));
> +	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> +
> +	ASSERT_EQ(0, close(socket_fds[0]));
> +	ASSERT_EQ(0, close(socket_fds[1]));
> +}
> +
>   /* clang-format off */
>   FIXTURE(layout1_bind) {};
>   /* clang-format on */

Re: [PATCH v8 8/9] samples/landlock: Extend sample tool to support LANDLOCK_ACCESS_FS_TRUNCATE

[Mickaël Salaün]

On 01/10/2022 17:49, Günther Noack wrote:
> 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>
> ---
>   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;

You can now base your patches on the current Linus' master branch, these 
three commits are now merged: 
https://git.kernel.org/mic/c/2fff00c81d4c37a037cf704d2d219fbcb45aea3c

The (inlined) documentation also needs to be updated according to this 
commit to align with the double backtick convention.


> +	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);

Re: [PATCH v8 9/9] landlock: Document Landlock's file truncation support

[Mickaël Salaün]

On 01/10/2022 17:49, 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 | 66 +++++++++++++++++++++---
>   1 file changed, 59 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index b8ea59493964..44d6f598b63d 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -8,7 +8,7 @@ Landlock: unprivileged access control
>   =====================================
>   
>   :Author: Mickaël Salaün
> -:Date: May 2022
> +:Date: October 2022
>   
>   The goal of Landlock is to enable to restrict ambient rights (e.g. global
>   filesystem access) for a set of processes.  Because Landlock is a stackable
> @@ -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,27 @@ 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`` or ``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) {
> +    if (abi < 0) {
> +        perror("The running kernel does not enable to use Landlock");

Please insert in a dedicated line this comment: /* Degrades gracefully 
if Landlock is not handled. */


> +        return 0;  /* Degrade gracefully if Landlock is not handled. */
> +    }
> +    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;
>       }
>   
>   This enables to create an inclusive ruleset that will contain our rules.
> @@ -127,8 +139,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 +263,37 @@ 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 require the
> +``LANDLOCK_ACCESS_FS_WRITE_FILE`` right.  Apart from the :manpage:`truncate(2)`
> +system call, this can also be done through :manpage:`open(2)` with the flags
> +``O_RDONLY | O_TRUNC``.
> +
> +When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
> +right is associated with the newly created file descriptor and will be used for
> +subsequent truncation attempts using :manpage:`ftruncate(2)`.  The behavior is
> +similar to opening a file for reading or writing, where permissions are checked
> +during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
> +:manpage:`write(2)` calls.
> +
> +As a consequence, it is possible to have multiple open file descriptors for the
> +same file, where one grants the right to truncate the file and the other does
> +not.  It is also possible to pass such file descriptors between processes,
> +keeping their Landlock properties, even when these processes do not have an
> +enforced Landlock ruleset.
> +
>   Compatibility
>   =============
>   
> @@ -397,6 +440,15 @@ Starting with the Landlock ABI version 2, it is now possible to securely
>   control renaming and linking thanks to the new `LANDLOCK_ACCESS_FS_REFER`
>   access right.
>   
> +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.
> +
>   .. _kernel_support:
>   
>   Kernel support

Re: [PATCH v8 0/9] landlock: truncate support

[Mickaël Salaün]

This patch series is almost ready, I guess the next one will be the 
final one.

I sent a related PR for syzkaller: 
https://github.com/google/syzkaller/pull/3423


On 01/10/2022 17:48, Günther Noack wrote:
> The goal of these patches is to work towards a more complete coverage
> of file system operations that are restrictable with Landlock.
> 
> Motivation
> ----------
> 
> 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.
> 
> 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.
> 
> Implementation overview
> -----------------------
> 
> 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.
> 
> Additionally, this patch set introduces a new Landlock security blob
> associated with opened files, to track the available Landlock access
> rights at the time of opening the file. This is in line with Unix's
> general approach of checking the read and write permissions during
> open(), and associating this previously checked authorization with the
> opened file.
> 
> In order to treat truncate(2) and ftruncate(2) calls differently in an
> LSM hook, we split apart the existing security_path_truncate hook into
> security_path_truncate (for truncation by path) and
> security_file_truncate (for truncation of previously opened files).
> 
> Relationship between "truncate" and "write" rights
> --------------------------------------------------
> 
> 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.
> 
> When the "truncate" right is checked for ftruncate(2)
> -----------------------------------------------------
> 
> Notably, for the purpose of ftruncate(2), the Landlock truncation
> access right is looked up when *opening* the file, not when calling
> ftruncate(). The availability of the truncate right is associated with
> the opened file and is later checked to authorize ftruncate(2)
> operations.
> 
> This is similar to how the write mode gets remembered after a
> open(..., O_WRONLY) to authorize later write() operations.
> 
> These opened file descriptors can also be passed between processes and
> will continue to enforce their truncation properties when these
> processes attempt an ftruncate().
> 
> Ongoing discussions
> -------------------
> 
> The one remaining ongoing discussion from v6 of the patch set is the
> question whether we need to touch fs/ksmbd and fs/cachefiles, which
> are both using vfs_truncate() to truncate files by path, even though
> they already have the same struct file open. The proposal was to
> introduce a "vfs_ftruncate()" that would work on opened files.
> 
> I think we should decouple this from the truncate patch set, with the
> reasoning that:
> 
> (a) it would be a bigger change to create a "vfs_ftruncate()" which
> would reach beyond the scope of this patch set.
> 
> (b) it seems likely that both components do not need to run under
> Landlock at the moment and can be updated independently (just like it
> needs to happen for normal userspace software in order to run it under
> Landlock).
> 
> (c) vfs_truncate() is not the perfectly narrowest API for truncating
> an opened file, but it's a legitimate way to do that and the operation
> *is* checked with a Landlock LSM hook, although it might potentially
> permit for a narrower sandboxing if that was done differently. That's
> speculative though.
> 
> Overall, it's unclear whether doing this has any sandboxing benefits
> for ksmbd and cachefiles, whereas on the downside, it would expand the
> scope of the patch set quite a bit and would have to touch core parts
> of the kernel (fs/open.c).
> 
> These patches are based on version 6.0-rc7.
> 
> Best regards,
> Günther
> 
> [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> 
> Past discussions:
> 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/
> V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
> V5: https://lore.kernel.org/all/20220817203006.21769-1-gnoack3000@gmail.com/
> V6: https://lore.kernel.org/all/20220908195805.128252-1-gnoack3000@gmail.com/
> V7: https://lore.kernel.org/all/20220930160144.141504-1-gnoack3000@gmail.com/
> 
> Changelog:
> 
> V8:
> * landlock: Refactor check_access_path_dual() into
>    is_access_to_paths_allowed(), as suggested by Mickaël Salaün on the
>    v7 review. Added this as a separate commit.
> * landlock truncate feature: inline get_path_access()
> * Documentation: update documentation date to October 2022
> * selftests: locally #define __maybe_unused (checkpatch started
>    complaining about it, but the header where it's defined is not
>    usable from selftests)
> 
> V7:
> * security: Create file_truncate hook
>    * Fix the build on kernels without CONFIG_SECURITY_PATH (fixed by
>      Mickaël Salaün)
>    * lsm_hooks.h: Document file_truncate hook
>    * fs/open.c: undo accidental indentation changes
> * landlock: Support file truncation
>    * Use the init_layer_masks() result as input for
>      check_access_path_dual()
>    * Naming
>      * Rename the landlock_file_security.allowed_access field
>        (previously called "rights")
>      * Rename get_path_access_rights() to get_path_access()
>      * Rename get_file_access() to get_required_file_open_access() to
>        avoid confusion with get_path_access()
>      * Use "access_request" for access_mask_t variables, access_req for
>        unsigned long
>    * Documentation:
>      * Fixed some comments according to review
>      * Added comments to get_required_file_open_access() and
>        init_layer_masks()
> * selftests:
>    * remove unused variables
>    * rename fd0,...,fd3 to fd_layer0,...,fd_layer3.
>    * test_ftruncate: define layers on top and inline the helper function
> * New tests (both added as separate commits)
>    * More exhaustive ftruncate test: Add open_and_ftruncate test that
>      exercises ftruncate more thoroughly with fixture variants
>    * FD-passing test: exercise restricted file descriptors getting
>      passed between processes, also using the same fixture variants
> * Documentation: integrate review comments by Mickaël Salaün
>    * do not use contraptions (don't)
>    * use double backquotes in all touched lines
>    * add the read/write open() analogy to the truncation docs
>    * in code example, check for abi<0 explicitly and fix indentation
> 
> V6:
> * LSM hooks: create file_truncate hook in addition to path_truncate.
>    Use it in the existing path_truncate call sites where appropriate.
> * landlock: check LANDLOCK_ACCESS_FS_TRUNCATE right during open(), and
>    associate that right with the opened struct file in a security blob.
>    Introduce get_path_access_rights() helper function.
> * selftests: test ftruncate in a separate test, to exercise that
>    the rights are associated with the file descriptor.
> * Documentation: Rework documentation to reflect new ftruncate() semantics.
> * Applied small fixes by Mickaël Salaün which he added on top of V5, in
>    https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
>    (I hope I found them all.)
> 
> V5:
> * Documentation
>    * Fix wording in userspace-api headers and in landlock.rst.
>    * Move the truncation limitation section one to the bottom.
>    * Move all .rst changes into the documentation commit.
> * selftests
>    * Remove _metadata argument from helpers where it became unnecessary.
>    * Open writable file descriptors at the top of both tests, before Landlock
>      is enabled, to exercise ftruncate() independently from open().
>    * Simplify test_ftruncate and decouple it from exercising open().
>    * test_creat(): Return errno on close() failure (it does not conflict).
>    * Fix /* comment style */
>    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
>    * Add missing |O_TRUNC to a check in one test.
>    * Put the truncate_unhandled test before the other.
> 
> 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.
> 
> Günther Noack (9):
>    security: Create file_truncate hook from path_truncate hook
>    selftests/landlock: Locally define __maybe_unused
>    landlock: Refactor check_access_path_dual() into
>      is_access_to_paths_allowed()
>    landlock: Support file truncation
>    selftests/landlock: Selftests for file truncation support
>    selftests/landlock: Test open() and ftruncate() in multiple scenarios
>    selftests/landlock: Test FD passing from a Landlock-restricted to an
>      unrestricted process
>    samples/landlock: Extend sample tool to support
>      LANDLOCK_ACCESS_FS_TRUNCATE
>    landlock: Document Landlock's file truncation support
> 
>   Documentation/userspace-api/landlock.rst     |  66 ++-
>   fs/namei.c                                   |   2 +-
>   fs/open.c                                    |   2 +-
>   include/linux/lsm_hook_defs.h                |   1 +
>   include/linux/lsm_hooks.h                    |  10 +-
>   include/linux/security.h                     |   6 +
>   include/uapi/linux/landlock.h                |  21 +-
>   samples/landlock/sandboxer.c                 |  23 +-
>   security/apparmor/lsm.c                      |   6 +
>   security/landlock/fs.c                       | 191 +++++---
>   security/landlock/fs.h                       |  24 +
>   security/landlock/limits.h                   |   2 +-
>   security/landlock/setup.c                    |   1 +
>   security/landlock/syscalls.c                 |   2 +-
>   security/security.c                          |   5 +
>   security/tomoyo/tomoyo.c                     |  13 +
>   tools/testing/selftests/landlock/base_test.c |  38 +-
>   tools/testing/selftests/landlock/common.h    |  85 +++-
>   tools/testing/selftests/landlock/fs_test.c   | 452 ++++++++++++++++++-
>   19 files changed, 828 insertions(+), 122 deletions(-)
> 
> 
> base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff

Re: [PATCH v8 1/9] security: Create file_truncate hook from path_truncate hook

[Paul Moore]

On Sat, Oct 1, 2022 at 11:49 AM Günther Noack <gnoack3000@gmail.com> wrote:
>
> Like path_truncate, the file_truncate hook also restricts file
> truncation, but is called in the cases where truncation is attempted
> on an already-opened file.
>
> This is required in a subsequent commit to handle ftruncate()
> operations differently to truncate() operations.
>
> Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>  fs/namei.c                    |  2 +-
>  fs/open.c                     |  2 +-
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/lsm_hooks.h     | 10 +++++++++-
>  include/linux/security.h      |  6 ++++++
>  security/apparmor/lsm.c       |  6 ++++++
>  security/security.c           |  5 +++++
>  security/tomoyo/tomoyo.c      | 13 +++++++++++++
>  8 files changed, 42 insertions(+), 3 deletions(-)

I agree with Mickaël's comments regarding the formatting, but
otherwise it looks okay to me from a LSM perspective.  If you make the
whitespace changes you can add my ACK.

Acked-by: Paul Moore <paul@paul-moore.com>

-- 
paul-moore.com

Re: [PATCH v8 4/9] landlock: Support file truncation

[Günther Noack]

On Wed, Oct 05, 2022 at 08:52:41PM +0200, Mickaël Salaün wrote:
> On 04/10/2022 21:56, Nathan Chancellor wrote:
> > Hi Günther,
> > 
> > On Sat, Oct 01, 2022 at 05:49:03PM +0200, 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 updates code documentation to document
> > > the flag.
> > > 
> > > The following operations are restricted:
> > > 
> > > open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
> > > implicitly truncated as part of the open() (e.g. using O_TRUNC).
> > > 
> > > Notable special cases:
> > > * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
> > > * open() with O_TRUNC does *not* need the TRUNCATE right when it
> > >    creates a new file.
> > > 
> > > truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
> > > right.
> > > 
> > > ftruncate() (on a file): requires that the file had the TRUNCATE right
> > > when it was previously opened.
> > > 
> > > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > 
> > I just bisected a crash in QEMU with Debian's arm64 configuration to
> > this change in -next as commit b40deebe7679 ("landlock: Support file
> > truncation"), which I was able to reproduce like so:
> 
> Thanks for the report Nathan. I've found an issue in this patch and fixed it
> in -next with this (rebased) commit:
> https://git.kernel.org/mic/c/b40deebe7679b05d4852488ef531e189a9621f2e
> You should already have this update since I pushed it Monday.
> Please let us know if this fixed the issue.

I'm afraid Nathan already tested it with the version from the mic
-next branch b40deebe7679 ("landlock: Support file truncation")

Nathan, thank you for the report and especially for the detailed step
by step instructions! I have tried to reproduce it with the steps you
suggested with the root file system you linked to, but I'm afraid I
was unable to reproduce the crash.

When I've tried it, the kernel boots up to init and shuts down again,
but does not run into the issue you're reporting:

...
[    1.165628] Run /init as init process
Starting syslogd: OK
Starting klogd: OK
Running sysctl: OK
Saving random seed: OK
Starting network: OK
Linux version 6.0.0-rc7-00004-gb40deebe7679 (gnoack@nuc) (aarch64-linux-gnu-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #4 SMP PREEMPT Thu Oct 6 21:45:59 CEST 2022
Stopping network: OK
Saving random seed: OK
Stopping klogd: OK
Stopping syslogd: OK
umount: devtmpfs busy - remounted read-only
umount: can't unmount /: Invalid argument
The system is going down NOW!
Sent SIGTERM to all processes
Sent SIGKILL to all processes
Requesting system poweroff
[    5.303758] kvm: exiting hardware virtualization
[    5.315878] Flash device refused suspend due to active operation (state 20)
[    5.318164] Flash device refused suspend due to active operation (state 20)
[    5.322274] reboot: Power down

I've been compiling the kernel using

make -j8 ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- O=build olddefconfig Image.gz

and started the emulator using

qemu-system-aarch64 -machine virt,gic-version=max,virtualization=true \
  -cpu max,pauth-impdef=true \
  -kernel /home/gnoack/linux/build/arch/arm64/boot/Image.gz \
  -append "console=ttyAMA0 earlycon" \
  -display none -m 512m -nodefaults -no-reboot -serial mon:stdio \
  -initrd /tmp/rootfs.cpio

I have attempted it with both the commit from the mic -next branch, as
well as with my own client that is still lacking Mickaël's LSM hook
fix. The commits I've tried are:

* b40deebe7679 ("landlock: Support file truncation")
* 224e66a32f16 ("landlock: Document Landlock's file truncation support")

I've built the kernel from scratch from the config file you suggested,
to be sure.

I used the rootfs.cpio file you provided from Github, and gcc 12.2.0
and qemu 7.1.0 from the Arch Linux repositories.

At this point, I don't know what else I can try to get it to crash...
it seems to work fine for me.

—Günther

> > 
> > $ mkdir -p build/deb
> > 
> > $ cd build/deb
> > 
> > $ curl -LSsO http://ftp.us.debian.org/debian/pool/main/l/linux-signed-arm64/linux-image-5.19.0-2-arm64_5.19.11-1_arm64.deb
> > 
> > $ ar x linux-image-5.19.0-2-arm64_5.19.11-1_arm64.deb
> > 
> > $ tar xJf data.tar.xz
> > 
> > $ cp boot/config-5.19.0-2-arm64 ../.config
> > 
> > $ cd ../..
> > 
> > $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- O=build olddefconfig Image.gz
> > 
> > $ qemu-system-aarch64 \
> > -machine virt,gic-version=max,virtualization=true \
> > -cpu max,pauth-impdef=true \
> > -kernel build/arch/arm64/boot/Image.gz \
> > -append "console=ttyAMA0 earlycon" \
> > -display none \
> > -initrd .../rootfs.cpio \
> > -m 512m \
> > -nodefaults \
> > -no-reboot \
> > -serial mon:stdio
> > ...
> > [    0.000000] Linux version 6.0.0-rc7+ (nathan@dev-arch.thelio-3990X) (aarch64-linux-gnu-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #1 SMP Tue Oct 4 12:48:50 MST 2022
> > ...
> > [    0.518570] Unable to handle kernel paging request at virtual address ffff00000851ff8a
> > [    0.518785] Mem abort info:
> > [    0.518867]   ESR = 0x0000000097c0c061
> > [    0.519001]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [    0.519155]   SET = 0, FnV = 0
> > [    0.519267]   EA = 0, S1PTW = 0
> > [    0.519386]   FSC = 0x21: alignment fault
> > [    0.519524] Data abort info:
> > [    0.519615]   Access size = 8 byte(s)
> > [    0.519722]   SSE = 0, SRT = 0
> > [    0.519817]   SF = 1, AR = 1
> > [    0.519920]   CM = 0, WnR = 1
> > [    0.520040] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000041711000
> > [    0.520225] [ffff00000851ff8a] pgd=180000005fff8003, p4d=180000005fff8003, pud=180000005fff7003, pmd=180000005ffbd003, pte=006800004851ff07
> > [    0.521121] Internal error: Oops: 97c0c061 [#1] SMP
> > [    0.521364] Modules linked in:
> > [    0.521592] CPU: 0 PID: 9 Comm: kworker/u2:0 Not tainted 6.0.0-rc7+ #1
> > [    0.521863] Hardware name: linux,dummy-virt (DT)
> > [    0.522325] Workqueue: events_unbound async_run_entry_fn
> > [    0.522973] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    0.523193] pc : apparmor_file_alloc_security+0x98/0x1e0
> > [    0.523431] lr : apparmor_file_alloc_security+0x48/0x1e0
> > [    0.523594] sp : ffff800008093960
> > [    0.523708] x29: ffff800008093960 x28: ffff800008093b30 x27: ffff000002602600
> > [    0.523978] x26: ffffd79796ecf8c0 x25: ffff00000241e705 x24: ffffd79797d98068
> > [    0.524199] x23: ffff00000851ff82 x22: ffff00000851ff80 x21: 0000000000000002
> > [    0.524431] x20: ffffd79796ff5000 x19: ffff00000241ceb0 x18: ffffffffffffffff
> > [    0.524647] x17: 000000000000003f x16: ffffd79797678008 x15: 0000000000000000
> > [    0.524850] x14: 0000000000000001 x13: 0000000000000000 x12: 0000000000000006
> > [    0.525087] x11: ffff00001feef940 x10: ffffd7979768f8a0 x9 : ffffd79796c1e51c
> > [    0.525325] x8 : ffff00000851ffa0 x7 : 0000000000000000 x6 : 0000000000001e0b
> > [    0.525531] x5 : ffff00000851ff80 x4 : ffff800008093990 x3 : ffff000002419700
> > [    0.525745] x2 : 0000000000000001 x1 : ffff00000851ff8a x0 : ffff00000241ceb0
> > [    0.526034] Call trace:
> > [    0.526166]  apparmor_file_alloc_security+0x98/0x1e0
> > [    0.526424]  security_file_alloc+0x6c/0xf0
> > [    0.526570]  __alloc_file+0x5c/0xf0
> > [    0.526699]  alloc_empty_file+0x68/0x10c
> > [    0.526816]  path_openat+0x50/0x106c
> > [    0.526929]  do_filp_open+0x88/0x13c
> > [    0.527041]  filp_open+0x110/0x1b0
> > [    0.527143]  do_name+0xbc/0x230
> > [    0.527256]  write_buffer+0x40/0x60
> > [    0.527359]  unpack_to_rootfs+0x100/0x2bc
> > [    0.527479]  do_populate_rootfs+0x70/0x134
> > [    0.527602]  async_run_entry_fn+0x40/0x1c0
> > [    0.527723]  process_one_work+0x1f4/0x450
> > [    0.527851]  worker_thread+0x188/0x4c0
> > [    0.527980]  kthread+0xe0/0xe4
> > [    0.528066]  ret_from_fork+0x10/0x20
> > [    0.528317] Code: 52800002 d2800000 d2800013 910022e1 (c89ffc20)
> > [    0.528736] ---[ end trace 0000000000000000 ]---
> > ...
> > 
> > A rootfs is available at [1] but I don't think it should be necessary
> > for reproducing this. If there is any additional information I can
> > provide or patches I can test, I am more than happy to do so!
> > 
> > [1]: https://github.com/ClangBuiltLinux/boot-utils/raw/bf2fd3500d87f78a914bfc3769b2240f5632e5b9/images/arm64/rootfs.cpio.zst
> > 
> > Cheers,
> > Nathan

-- 

Re: [PATCH v8 1/9] security: Create file_truncate hook from path_truncate hook

[Günther Noack]

On Wed, Oct 05, 2022 at 08:53:35PM +0200, Mickaël Salaün wrote:
> Thanks for the doc.
> 
> On 01/10/2022 17:49, Günther Noack wrote:
> > Like path_truncate, the file_truncate hook also restricts file
> > truncation, but is called in the cases where truncation is attempted
> > on an already-opened file.
> > 
> > This is required in a subsequent commit to handle ftruncate()
> > operations differently to truncate() operations.
> > 
> > Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Acked-by: John Johansen <john.johansen@canonical.com>
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   fs/namei.c                    |  2 +-
> >   fs/open.c                     |  2 +-
> >   include/linux/lsm_hook_defs.h |  1 +
> >   include/linux/lsm_hooks.h     | 10 +++++++++-
> >   include/linux/security.h      |  6 ++++++
> >   security/apparmor/lsm.c       |  6 ++++++
> >   security/security.c           |  5 +++++
> >   security/tomoyo/tomoyo.c      | 13 +++++++++++++
> >   8 files changed, 42 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 53b4bc094db2..0e419bd30f8e 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
> >   	if (error)
> >   		return error;
> > -	error = security_path_truncate(path);
> > +	error = security_file_truncate(filp);
> >   	if (!error) {
> >   		error = do_truncate(mnt_userns, path->dentry, 0,
> >   				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> > diff --git a/fs/open.c b/fs/open.c
> > index cf7e5c350a54..0fa861873245 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> >   	if (IS_APPEND(file_inode(f.file)))
> >   		goto out_putf;
> >   	sb_start_write(inode->i_sb);
> > -	error = security_path_truncate(&f.file->f_path);
> > +	error = security_file_truncate(f.file);
> >   	if (!error)
> >   		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
> >   				    ATTR_MTIME | ATTR_CTIME, f.file);
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 60fff133c0b1..dee35ab253ba 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
> >   	 struct fown_struct *fown, int sig)
> >   LSM_HOOK(int, 0, file_receive, struct file *file)
> >   LSM_HOOK(int, 0, file_open, struct file *file)
> > +LSM_HOOK(int, 0, file_truncate, struct file *file)
> >   LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
> >   	 unsigned long clone_flags)
> >   LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 3aa6030302f5..4acc975f28d9 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -409,7 +409,9 @@
> >    *	@attr is the iattr structure containing the new file attributes.
> >    *	Return 0 if permission is granted.
> >    * @path_truncate:
> > - *	Check permission before truncating a file.
> > + *	Check permission before truncating the file indicated by path.
> > + *      Note that truncation permissions may also be checked based on
> > + *      already opened files, using the @file_truncate hook.
> 
> The documentation comments (mostly) use tabs, not spaces.

Oops, well spotted. Done.

> >    *	@path contains the path structure for the file.
> >    *	Return 0 if permission is granted.
> >    * @inode_getattr:
> > @@ -598,6 +600,12 @@
> >    *	to receive an open file descriptor via socket IPC.
> >    *	@file contains the file structure being received.
> >    *	Return 0 if permission is granted.
> > + * @file_truncate:
> > + *	Check permission before truncating a file, i.e. using ftruncate.
> > + *	Note that truncation permission may also be checked based on the path,
> > + *      using the @path_truncate hook.
> 
> Same here.

Done.

> > + *	@file contains the file structure for the file.
> > + *	Return 0 if permission is granted.
> >    * @file_open:
> >    *	Save open-time permission checking state for later use upon
> >    *	file_permission, and recheck access if anything has changed
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 7bd0c490703d..f80b23382dd9 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
> >   				 struct fown_struct *fown, int sig);
> >   int security_file_receive(struct file *file);
> >   int security_file_open(struct file *file);
> > +int security_file_truncate(struct file *file);
> >   int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
> >   void security_task_free(struct task_struct *task);
> >   int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> > @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
> >   	return 0;
> >   }
> > +static inline int security_file_truncate(struct file *file)
> > +{
> > +	return 0;
> > +}
> > +
> >   static inline int security_task_alloc(struct task_struct *task,
> >   				      unsigned long clone_flags)
> >   {
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index e29cade7b662..98ecb7f221b8 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
> >   	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
> >   }
> > +static int apparmor_file_truncate(struct file *file)
> > +{
> > +	return apparmor_path_truncate(&file->f_path);
> > +}
> > +
> >   static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
> >   				 const char *old_name)
> >   {
> > @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> >   	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
> >   	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
> >   	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
> > +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
> >   	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
> >   	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
> > diff --git a/security/security.c b/security/security.c
> > index 4b95de24bc8d..d73e423005c3 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1650,6 +1650,11 @@ int security_file_open(struct file *file)
> >   	return fsnotify_perm(file, MAY_OPEN);
> >   }
> > +int security_file_truncate(struct file *file)
> > +{
> > +	return call_int_hook(file_truncate, 0, file);
> > +}
> > +
> >   int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
> >   {
> >   	int rc = lsm_task_alloc(task);
> > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> > index 71e82d855ebf..af04a7b7eb28 100644
> > --- a/security/tomoyo/tomoyo.c
> > +++ b/security/tomoyo/tomoyo.c
> > @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
> >   	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
> >   }
> > +/**
> > + * tomoyo_file_truncate - Target for security_file_truncate().
> > + *
> > + * @file: Pointer to "struct file".
> > + *
> > + * Returns 0 on success, negative value otherwise.
> > + */
> > +static int tomoyo_file_truncate(struct file *file)
> > +{
> > +	return tomoyo_path_truncate(&file->f_path);
> > +}
> > +
> >   /**
> >    * tomoyo_path_unlink - Target for security_path_unlink().
> >    *
> > @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
> >   	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
> >   	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
> >   	LSM_HOOK_INIT(file_open, tomoyo_file_open),
> > +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
> >   	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
> >   	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
> >   	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),

-- 

Re: [PATCH v8 2/9] selftests/landlock: Locally define __maybe_unused

[Günther Noack]

On Wed, Oct 05, 2022 at 08:53:56PM +0200, Mickaël Salaün wrote:
> Thanks for cleaning this. Can you please move this patch just before the
> test patches?

Done.

-- 

Re: [PATCH v8 3/9] landlock: Refactor check_access_path_dual() into is_access_to_paths_allowed()

[Günther Noack]

On Wed, Oct 05, 2022 at 08:54:08PM +0200, Mickaël Salaün wrote:
> On 01/10/2022 17:49, Günther Noack wrote:
> > * Rename it to is_access_to_paths_allowed()
> > * Make it return true iff the access is allowed
> > * Calculate the EXDEV/EACCES error code in the one place where it's needed
> 
> Can you please replace these bullet points with (one-sentence) paragraphs?

Done.

> > Suggested-by: Mickaël Salaün <mic@digikod.net>
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   security/landlock/fs.c | 89 +++++++++++++++++++++---------------------
> >   1 file changed, 44 insertions(+), 45 deletions(-)
> > 
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index a9dbd99d9ee7..083dd3d359de 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -430,7 +430,7 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
> >   }
> >   /**
> > - * check_access_path_dual - Check accesses for requests with a common path
> > + * is_access_to_paths_allowed - Check accesses for requests with a common path
> >    *
> >    * @domain: Domain to check against.
> >    * @path: File hierarchy to walk through.
> > @@ -465,14 +465,10 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
> >    * allow the request.
> >    *
> >    * Returns:
> > - * - 0 if the access request is granted;
> > - * - -EACCES if it is denied because of access right other than
> > - *   LANDLOCK_ACCESS_FS_REFER;
> > - * - -EXDEV if the renaming or linking would be a privileged escalation
> > - *   (according to each layered policies), or if LANDLOCK_ACCESS_FS_REFER is
> > - *   not allowed by the source or the destination.
> > + * - true if the access request is granted;
> > + * - false otherwise
> 
> Missing final dot.

Done.

> >    */
> > -static int check_access_path_dual(
> > +static bool is_access_to_paths_allowed(
> >   	const struct landlock_ruleset *const domain,
> >   	const struct path *const path,
> >   	const access_mask_t access_request_parent1,
> > @@ -492,17 +488,17 @@ static int check_access_path_dual(
> >   	(*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL;
> >   	if (!access_request_parent1 && !access_request_parent2)
> > -		return 0;
> > +		return true;
> >   	if (WARN_ON_ONCE(!domain || !path))
> > -		return 0;
> > +		return true;
> >   	if (is_nouser_or_private(path->dentry))
> > -		return 0;
> > +		return true;
> >   	if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1))
> > -		return -EACCES;
> > +		return false;
> >   	if (unlikely(layer_masks_parent2)) {
> >   		if (WARN_ON_ONCE(!dentry_child1))
> > -			return -EACCES;
> > +			return false;
> >   		/*
> >   		 * For a double request, first check for potential privilege
> >   		 * escalation by looking at domain handled accesses (which are
> > @@ -513,7 +509,7 @@ static int check_access_path_dual(
> >   		is_dom_check = true;
> >   	} else {
> >   		if (WARN_ON_ONCE(dentry_child1 || dentry_child2))
> > -			return -EACCES;
> > +			return false;
> >   		/* For a simple request, only check for requested accesses. */
> >   		access_masked_parent1 = access_request_parent1;
> >   		access_masked_parent2 = access_request_parent2;
> > @@ -622,24 +618,7 @@ static int check_access_path_dual(
> >   	}
> >   	path_put(&walker_path);
> > -	if (allowed_parent1 && allowed_parent2)
> > -		return 0;
> > -
> > -	/*
> > -	 * This prioritizes EACCES over EXDEV for all actions, including
> > -	 * renames with RENAME_EXCHANGE.
> > -	 */
> > -	if (likely(is_eacces(layer_masks_parent1, access_request_parent1) ||
> > -		   is_eacces(layer_masks_parent2, access_request_parent2)))
> > -		return -EACCES;
> > -
> > -	/*
> > -	 * Gracefully forbids reparenting if the destination directory
> > -	 * hierarchy is not a superset of restrictions of the source directory
> > -	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
> > -	 * source or the destination.
> > -	 */
> > -	return -EXDEV;
> > +	return allowed_parent1 && allowed_parent2;
> >   }
> >   static inline int check_access_path(const struct landlock_ruleset *const domain,
> > @@ -649,8 +628,10 @@ static inline int check_access_path(const struct landlock_ruleset *const domain,
> >   	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> >   	access_request = init_layer_masks(domain, access_request, &layer_masks);
> > -	return check_access_path_dual(domain, path, access_request,
> > -				      &layer_masks, NULL, 0, NULL, NULL);
> > +	if (is_access_to_paths_allowed(domain, path, access_request,
> > +				       &layer_masks, NULL, 0, NULL, NULL))
> > +		return 0;
> > +	return -EACCES;
> >   }
> >   static inline int current_check_access_path(const struct path *const path,
> > @@ -711,8 +692,9 @@ static inline access_mask_t maybe_remove(const struct dentry *const dentry)
> >    * file.  While walking from @dir to @mnt_root, we record all the domain's
> >    * allowed accesses in @layer_masks_dom.
> >    *
> > - * This is similar to check_access_path_dual() but much simpler because it only
> > - * handles walking on the same mount point and only check one set of accesses.
> > + * This is similar to is_access_to_paths_allowed() but much simpler because it
> > + * only handles walking on the same mount point and only checks one set of
> > + * accesses.
> >    *
> >    * Returns:
> >    * - true if all the domain access rights are allowed for @dir;
> > @@ -857,10 +839,11 @@ static int current_check_refer_path(struct dentry *const old_dentry,
> >   		access_request_parent1 = init_layer_masks(
> >   			dom, access_request_parent1 | access_request_parent2,
> >   			&layer_masks_parent1);
> > -		return check_access_path_dual(dom, new_dir,
> > -					      access_request_parent1,
> > -					      &layer_masks_parent1, NULL, 0,
> > -					      NULL, NULL);
> > +		if (is_access_to_paths_allowed(
> > +			    dom, new_dir, access_request_parent1,
> > +			    &layer_masks_parent1, NULL, 0, NULL, NULL))
> > +			return 0;
> > +		return -EACCES;
> >   	}
> >   	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
> > @@ -886,11 +869,27 @@ static int current_check_refer_path(struct dentry *const old_dentry,
> >   	 * parent access rights.  This will be useful to compare with the
> >   	 * destination parent access rights.
> >   	 */
> > -	return check_access_path_dual(dom, &mnt_dir, access_request_parent1,
> > -				      &layer_masks_parent1, old_dentry,
> > -				      access_request_parent2,
> > -				      &layer_masks_parent2,
> > -				      exchange ? new_dentry : NULL);
> > +	if (is_access_to_paths_allowed(
> > +		    dom, &mnt_dir, access_request_parent1, &layer_masks_parent1,
> > +		    old_dentry, access_request_parent2, &layer_masks_parent2,
> > +		    exchange ? new_dentry : NULL))
> > +		return 0;
> > +
> > +	/*
> > +	 * This prioritizes EACCES over EXDEV for all actions, including
> > +	 * renames with RENAME_EXCHANGE.
> > +	 */
> > +	if (likely(is_eacces(&layer_masks_parent1, access_request_parent1) ||
> > +		   is_eacces(&layer_masks_parent2, access_request_parent2)))
> > +		return -EACCES;
> > +
> > +	/*
> > +	 * Gracefully forbids reparenting if the destination directory
> > +	 * hierarchy is not a superset of restrictions of the source directory
> > +	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
> > +	 * source or the destination.
> > +	 */
> > +	return -EXDEV;
> >   }
> >   /* Inode hooks */

-- 

Re: [PATCH v8 4/9] landlock: Support file truncation

[Günther Noack]

On Wed, Oct 05, 2022 at 08:55:42PM +0200, Mickaël Salaün wrote:
> 
> On 01/10/2022 17:49, 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 updates code documentation to document
> > the flag.
> > 
> > The following operations are restricted:
> > 
> > open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
> > implicitly truncated as part of the open() (e.g. using O_TRUNC).
> > 
> > Notable special cases:
> > * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
> > * open() with O_TRUNC does *not* need the TRUNCATE right when it
> >    creates a new file.
> > 
> > truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
> > right.
> > 
> > ftruncate() (on a file): requires that the file had the TRUNCATE right
> > when it was previously opened.
> > 
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   include/uapi/linux/landlock.h                |  21 +++-
> >   security/landlock/fs.c                       | 102 +++++++++++++++++--
> >   security/landlock/fs.h                       |  24 +++++
> >   security/landlock/limits.h                   |   2 +-
> >   security/landlock/setup.c                    |   1 +
> >   security/landlock/syscalls.c                 |   2 +-
> >   tools/testing/selftests/landlock/base_test.c |   2 +-
> >   tools/testing/selftests/landlock/fs_test.c   |   7 +-
> >   8 files changed, 144 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..d830cdfdbe56 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -95,8 +95,19 @@ 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
> 
> %LANDLOCK_ACCESS_FS_TRUNCATE

Done.

> > + *   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 with :manpage:`truncate(2)`,
> > + *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
> > + *   `O_TRUNC`. Whether an opened file can be truncated with
> 
> %O_TRUNC

Done. (Also in the case a few line above.)


> > + *   :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
> > + *   same way as read and write permissions are checked during
> > + *   :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
> > + *   %LANDLOCK_ACCESS_FS_WRITE_FILE. 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 +150,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 +170,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 083dd3d359de..80d507ce2305 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 */
> >   /*
> > @@ -297,6 +298,18 @@ get_handled_accesses(const struct landlock_ruleset *const domain)
> >   	return access_dom & LANDLOCK_MASK_ACCESS_FS;
> >   }
> > +/*
> > + * init_layer_masks - Populates @layer_masks such that for each access right in
> > + * @access_request, the bits for all the layers are set where this access right
> > + * is handled.
> 
> Thanks for this extra documentation!
> 
> Can you convert it to a proper code documentation (even if it not used yet),
> with a heading `/**` and a short title following the function name?
> Something like "init_layer_masks - Initialize layer masks". Please follow
> this convention for the other doc strings, or just use a paragraph in a
> simple comment (e.g. for get_required_file_open_access).
> 
> Because there is no direct link with Landlock supporting truncation, this
> should be in a standalone patch, but you can keep it in this series.

Done, I split it off into a separate patch and fixed the formatting to
use /** and the right documentation format.

I fixed up the get_required_file_open_access documentation as well,
but kept it in this patch, because it's a related change.

> > + *
> > + * @domain: The domain that defines the current restrictions.
> > + * @access_request: The requested access rights to check.
> > + * @layer_masks: The layer masks to populate.
> > + *
> > + * Returns: An access mask where each access right bit is set which is handled
> > + * in any of the active layers in @domain.
> > + */
> >   static inline access_mask_t
> >   init_layer_masks(const struct landlock_ruleset *const domain,
> >   		 const access_mask_t access_request,
> > @@ -1141,9 +1154,19 @@ 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)
> > +/*
> > + * get_required_file_open_access - Returns the access rights that are required
> > + * for opening the file, depending on the file type and open mode.
> > + */
> > +static inline access_mask_t
> > +get_required_file_open_access(const struct file *const file)
> >   {
> >   	access_mask_t access = 0;
> > @@ -1163,17 +1186,82 @@ static inline access_mask_t get_file_access(const struct file *const file)
> >   static int hook_file_open(struct file *const file)
> >   {
> > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > +	access_mask_t open_access_request, full_access_request, allowed_access;
> > +	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> >   	const struct landlock_ruleset *const dom =
> >   		landlock_get_current_domain();
> > -	if (!dom)
> > +	if (!dom) {
> > +		/*
> > +		 * Grants all access rights, even if most of them are not
> > +		 * checked later on. It is more consistent.
> > +		 */
> > +		landlock_file(file)->allowed_access = LANDLOCK_MASK_ACCESS_FS;
> 
> This looks like the right approach but unfortunately, because there is
> multiple ways to get a file descriptors (e.g. memfd_create, which is worth
> mentioning in a comment), this doesn't work well. For now, it only makes
> sense for Landlock to restrict file descriptors obtained through open(2). We
> can then move this initialization to a new hook implementation for
> file_alloc_security.

Good catch -- that was indeed still a bug.

> I think this is the bug Nathan reported.

As discussed on the other thread, that is unclear, and I failed to
reproduce his crash here. He did try it with the fixed version from
your next branch.

> We should have a test with memfd_create(2) to make sure it works as
> expected. I think the documentation is still correct though.

I added the memfd_create(2) test in a separate commit, verified that
it fails with the old version of the code, as expected, and fixed the
bug in this commit with the same approach which you also took on your
next branch, by implementing a file_alloc_security hook.

> >   		return 0;
> > +	}
> > +
> >   	/*
> > -	 * Because a file may be opened with O_PATH, get_file_access() may
> > -	 * return 0.  This case will be handled with a future Landlock
> > +	 * Because a file may be opened with O_PATH, get_required_file_open_access()
> > +	 * may return 0.  This case will be handled with a future Landlock
> >   	 * evolution.
> >   	 */
> > -	return check_access_path(dom, &file->f_path, get_file_access(file));
> > +	open_access_request = get_required_file_open_access(file);
> > +
> > +	/*
> > +	 * We look up more access than what we immediately need for open(), so
> > +	 * that we can later authorize operations on opened files.
> > +	 */
> > +	full_access_request = open_access_request | optional_access;
> > +
> > +	allowed_access = full_access_request;
> > +	if (!is_access_to_paths_allowed(
> > +		    dom, &file->f_path,
> > +		    init_layer_masks(dom, full_access_request, &layer_masks),
> > +		    &layer_masks, NULL, 0, NULL, NULL)) {
> 
> I'd prefer (less error prone and easier to read) to add an
> is_access_paths_allowed branch to initialize allowed_access with
> full_access_request, and tweak this branch to initialize allowed_access with
> 0 and then populate it according to !layer_masks[access_bit].

Done, this makes sense.

> > +		unsigned long access_bit;
> > +		unsigned long access_req = full_access_request;
> 
> const unsigned long access_req

Done.

> > +
> > +		/*
> > +		 * Calculate the actual allowed access rights from layer_masks.
> > +		 * Remove each access right from allowed_access which has been
> > +		 * vetoed by any layer.
> > +		 */
> > +		for_each_set_bit(access_bit, &access_req,
> > +				 ARRAY_SIZE(layer_masks)) {
> > +			if (layer_masks[access_bit])
> > +				allowed_access &= ~BIT_ULL(access_bit); > +		}
> > +	}
> 
> We can move `landlock_file(file)->allowed_access = allowed_access` here to
> be sure that the struct file allowed access is consistent even if it should
> not be used (because access may be denied).

Done, moved it after the "if" branch to clarify that it happens unconditionally.

> > +
> > +	if (open_access_request & ~allowed_access)
> > +		return -EACCES;
> 
> And here invert the check ((open_access_request & allowed_access) ==
> open_access_request) to make it more consistent with other checks…

Done, good suggestion. I find this easier to read as well.

> > +
> > +	/*
> > +	 * For operations on already opened files (i.e. ftruncate()), it is the
> > +	 * access rights at the time of open() which decide whether the
> > +	 * operation is permitted. Therefore, we record the relevant subset of
> > +	 * file access rights in the opened struct file.
> > +	 */
> > +	landlock_file(file)->allowed_access = allowed_access;
> > +	return 0;
> 
> …and return -EACCES here.

Done.

> > +}
> > +
> > +static int hook_file_truncate(struct file *const file)
> > +{
> > +	/*
> > +	 * Allows truncation if the truncate right was available at the time of
> > +	 * opening the file, to get a consistent access check as for read, write
> > +	 * and execute operations.
> > +	 *
> > +	 * Note: For checks done based on the file's Landlock rights, we enforce
> 
> s/file's Landlock rights/file's Landlock allowed access/ maybe?

Done.

> > +	 * them independently of whether the current thread is in a Landlock
> > +	 * domain, so that open files passed between independent processes
> > +	 * retain their behaviour.
> > +	 */
> > +	if (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_TRUNCATE)
> > +		return 0;
> > +	return -EACCES;
> >   }
> >   static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> > @@ -1193,6 +1281,8 @@ 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_truncate, hook_file_truncate),
> 
> Please move the hook_file_truncate entry after the hook_file_open one, these
> entries are in the same order as their hook implementations.

Done.

> >   	LSM_HOOK_INIT(file_open, hook_file_open),
> >   };
> > diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> > index 8db7acf9109b..488e4813680a 100644
> > --- a/security/landlock/fs.h
> > +++ b/security/landlock/fs.h
> > @@ -36,6 +36,24 @@ struct landlock_inode_security {
> >   	struct landlock_object __rcu *object;
> >   };
> > +/**
> > + * struct landlock_file_security - File security blob
> > + *
> > + * This information is populated when opening a file in hook_file_open, and
> > + * tracks the relevant Landlock access rights that were available at the time
> > + * of opening the file. Other LSM hooks use these rights in order to authorize
> > + * operations on already opened files.
> > + */
> > +struct landlock_file_security {
> > +	/**
> > +	 * @allowed_access: Access rights that were available at the time of
> > +	 * opening the file. This is not necessarily the full set of access
> > +	 * rights available at that time, but it's the necessary subset as
> > +	 * needed to authorize later operations on the open file.
> > +	 */
> > +	access_mask_t allowed_access;
> > +};
> > +
> >   /**
> >    * struct landlock_superblock_security - Superblock security blob
> >    *
> > @@ -50,6 +68,12 @@ struct landlock_superblock_security {
> >   	atomic_long_t inode_refs;
> >   };
> > +static inline struct landlock_file_security *
> > +landlock_file(const struct file *const file)
> > +{
> > +	return file->f_security + landlock_blob_sizes.lbs_file;
> > +}
> > +
> >   static inline struct landlock_inode_security *
> >   landlock_inode(const struct inode *const inode)
> >   {
> > 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/setup.c b/security/landlock/setup.c
> > index f8e8e980454c..3f196d2ce4f9 100644
> > --- a/security/landlock/setup.c
> > +++ b/security/landlock/setup.c
> > @@ -19,6 +19,7 @@ bool landlock_initialized __lsm_ro_after_init = false;
> >   struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
> >   	.lbs_cred = sizeof(struct landlock_cred_security),
> > +	.lbs_file = sizeof(struct landlock_file_security),
> >   	.lbs_inode = sizeof(struct landlock_inode_security),
> >   	.lbs_superblock = sizeof(struct landlock_superblock_security),
> >   };
> > 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 45de42a027c5..87b28d14a1aa 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -406,9 +406,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 | \
> > @@ -422,7 +423,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 v8 7/9] selftests/landlock: Test FD passing from a Landlock-restricted to an unrestricted process

[Günther Noack]

On Wed, Oct 05, 2022 at 08:57:12PM +0200, Mickaël Salaün wrote:
> Thanks for this refactoring.
> 
> We need a shorter subject though. ;)

Done, changed to:
"selftests/landlock: Test FD passing from restricted to unrestricted processes"

> On 01/10/2022 17:49, Günther Noack wrote:
> > A file descriptor created in a restricted process carries Landlock
> > restrictions with it which will apply even if the same opened file is
> > used from an unrestricted process.
> > 
> > This change extracts suitable FD-passing helpers from base_test.c and
> > moves them to common.h. We use the fixture variants from the ftruncate
> > fixture to exercise the same scenarios as in the open_and_ftruncate
> > test, but doing the Landlock restriction and open() in a different
> > process than the ftruncate() call.
> > 
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   tools/testing/selftests/landlock/base_test.c | 36 +----------
> >   tools/testing/selftests/landlock/common.h    | 67 ++++++++++++++++++++
> >   tools/testing/selftests/landlock/fs_test.c   | 62 ++++++++++++++++++
> >   3 files changed, 132 insertions(+), 33 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> > index 72cdae277b02..6d1b6eedb432 100644
> > --- a/tools/testing/selftests/landlock/base_test.c
> > +++ b/tools/testing/selftests/landlock/base_test.c
> > @@ -263,23 +263,6 @@ TEST(ruleset_fd_transfer)
> >   		.allowed_access = LANDLOCK_ACCESS_FS_READ_DIR,
> >   	};
> >   	int ruleset_fd_tx, dir_fd;
> > -	union {
> > -		/* Aligned ancillary data buffer. */
> > -		char buf[CMSG_SPACE(sizeof(ruleset_fd_tx))];
> > -		struct cmsghdr _align;
> > -	} cmsg_tx = {};
> > -	char data_tx = '.';
> > -	struct iovec io = {
> > -		.iov_base = &data_tx,
> > -		.iov_len = sizeof(data_tx),
> > -	};
> > -	struct msghdr msg = {
> > -		.msg_iov = &io,
> > -		.msg_iovlen = 1,
> > -		.msg_control = &cmsg_tx.buf,
> > -		.msg_controllen = sizeof(cmsg_tx.buf),
> > -	};
> > -	struct cmsghdr *cmsg;
> >   	int socket_fds[2];
> >   	pid_t child;
> >   	int status;
> > @@ -298,33 +281,20 @@ TEST(ruleset_fd_transfer)
> >   				    &path_beneath_attr, 0));
> >   	ASSERT_EQ(0, close(path_beneath_attr.parent_fd));
> > -	cmsg = CMSG_FIRSTHDR(&msg);
> > -	ASSERT_NE(NULL, cmsg);
> > -	cmsg->cmsg_len = CMSG_LEN(sizeof(ruleset_fd_tx));
> > -	cmsg->cmsg_level = SOL_SOCKET;
> > -	cmsg->cmsg_type = SCM_RIGHTS;
> > -	memcpy(CMSG_DATA(cmsg), &ruleset_fd_tx, sizeof(ruleset_fd_tx));
> > -
> >   	/* Sends the ruleset FD over a socketpair and then close it. */
> >   	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
> >   				socket_fds));
> > -	ASSERT_EQ(sizeof(data_tx), sendmsg(socket_fds[0], &msg, 0));
> > +	ASSERT_EQ(0, send_fd(socket_fds[0], ruleset_fd_tx));
> >   	ASSERT_EQ(0, close(socket_fds[0]));
> >   	ASSERT_EQ(0, close(ruleset_fd_tx));
> >   	child = fork();
> >   	ASSERT_LE(0, child);
> >   	if (child == 0) {
> > -		int ruleset_fd_rx;
> > +		int ruleset_fd_rx = recv_fd(socket_fds[1]);
> 
> We can now make it const.

Done.

> > -		*(char *)msg.msg_iov->iov_base = '\0';
> > -		ASSERT_EQ(sizeof(data_tx),
> > -			  recvmsg(socket_fds[1], &msg, MSG_CMSG_CLOEXEC));
> > -		ASSERT_EQ('.', *(char *)msg.msg_iov->iov_base);
> > +		ASSERT_LE(0, ruleset_fd_rx);
> >   		ASSERT_EQ(0, close(socket_fds[1]));
> > -		cmsg = CMSG_FIRSTHDR(&msg);
> > -		ASSERT_EQ(cmsg->cmsg_len, CMSG_LEN(sizeof(ruleset_fd_tx)));
> > -		memcpy(&ruleset_fd_rx, CMSG_DATA(cmsg), sizeof(ruleset_fd_tx));
> >   		/* Enforces the received ruleset on the child. */
> >   		ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> > diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> > index 7d34592471db..15149d34e43b 100644
> > --- a/tools/testing/selftests/landlock/common.h
> > +++ b/tools/testing/selftests/landlock/common.h
> > @@ -10,6 +10,7 @@
> >   #include <errno.h>
> >   #include <linux/landlock.h>
> >   #include <sys/capability.h>
> > +#include <sys/socket.h>
> >   #include <sys/syscall.h>
> >   #include <sys/types.h>
> >   #include <sys/wait.h>
> > @@ -189,3 +190,69 @@ static void __maybe_unused clear_cap(struct __test_metadata *const _metadata,
> >   {
> >   	_effective_cap(_metadata, caps, CAP_CLEAR);
> >   }
> > +
> > +/* Receives an FD from a UNIX socket. Returns the received FD, -1 on error. */
> > +static int __maybe_unused recv_fd(int usock)
> > +{
> > +	int fd_rx;
> > +	union {
> > +		/* Aligned ancillary data buffer. */
> > +		char buf[CMSG_SPACE(sizeof(fd_rx))];
> > +		struct cmsghdr _align;
> > +	} cmsg_rx = {};
> > +	char data = '\0';
> > +	struct iovec io = {
> > +		.iov_base = &data,
> > +		.iov_len = sizeof(data),
> > +	};
> > +	struct msghdr msg = {
> > +		.msg_iov = &io,
> > +		.msg_iovlen = 1,
> > +		.msg_control = &cmsg_rx.buf,
> > +		.msg_controllen = sizeof(cmsg_rx.buf),
> > +	};
> > +	struct cmsghdr *cmsg;
> > +	int res;
> > +
> > +	res = recvmsg(usock, &msg, MSG_CMSG_CLOEXEC);
> > +	if (res < 0)
> > +		return -1;
> 
> It could be useful to return -errno for recv_fd() and send_fd(), and update
> the description accordingly. That would enable to quickly know the error
> with the ASSERT_*() calls.

Done, good idea.


> > +	cmsg = CMSG_FIRSTHDR(&msg);
> > +	if (cmsg->cmsg_len != CMSG_LEN(sizeof(fd_rx)))
> > +		return -1;

I made it return -EIO in the case where the other side sends invalid data.


> > +	memcpy(&fd_rx, CMSG_DATA(cmsg), sizeof(fd_rx));
> > +	return fd_rx;
> > +}
> > +
> > +/* Sends an FD on a UNIX socket. Returns 0 on success or -1 on error. */
> > +static int __maybe_unused send_fd(int usock, int fd_tx)
> > +{
> > +	union {
> > +		/* Aligned ancillary data buffer. */
> > +		char buf[CMSG_SPACE(sizeof(fd_tx))];
> > +		struct cmsghdr _align;
> > +	} cmsg_tx = {};
> > +	char data_tx = '.';
> > +	struct iovec io = {
> > +		.iov_base = &data_tx,
> > +		.iov_len = sizeof(data_tx),
> > +	};
> > +	struct msghdr msg = {
> > +		.msg_iov = &io,
> > +		.msg_iovlen = 1,
> > +		.msg_control = &cmsg_tx.buf,
> > +		.msg_controllen = sizeof(cmsg_tx.buf),
> > +	};
> > +	struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
> > +
> > +	cmsg->cmsg_len = CMSG_LEN(sizeof(fd_tx));
> > +	cmsg->cmsg_level = SOL_SOCKET;
> > +	cmsg->cmsg_type = SCM_RIGHTS;
> > +	memcpy(CMSG_DATA(cmsg), &fd_tx, sizeof(fd_tx));
> > +
> > +	if (sendmsg(usock, &msg, 0) < 0)
> > +		return -1;
> > +	return 0;
> > +}
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index 308f6f36e8c0..93ed80a25a74 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -3541,6 +3541,68 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> >   	}
> >   }
> > +TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
> > +{
> > +	int child, fd, status;
> > +	int socket_fds[2];
> > +
> > +	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
> > +				socket_fds));
> > +
> > +	child = fork();
> > +	ASSERT_LE(0, child);
> > +	if (child == 0) {
> > +		/*
> > +		 * Enable Landlock in the child process, open a file descriptor
> 
> "Enables"…

Done.

> > +		 * where truncation is forbidden and send it to the
> > +		 * non-landlocked parent process.
> > +		 */
> > +		const char *const path = file1_s1d1;
> > +		const struct rule rules[] = {
> > +			{
> > +				.path = path,
> > +				.access = variant->permitted,
> > +			},
> > +			{},
> > +		};
> > +		int fd, ruleset_fd;
> > +
> > +		/* Enable Landlock. */
> 
> This comment is not necessary.

Done.

> > +		ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > +		ASSERT_LE(0, ruleset_fd);
> > +		enforce_ruleset(_metadata, ruleset_fd);
> > +		ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +		fd = open(path, O_WRONLY);
> > +		ASSERT_EQ(variant->expected_open_result, (fd < 0 ? errno : 0));
> > +
> > +		if (fd >= 0) {
> > +			ASSERT_EQ(0, send_fd(socket_fds[0], fd));
> > +			ASSERT_EQ(0, close(fd));
> > +		}
> > +
> > +		ASSERT_EQ(0, close(socket_fds[0]));
> > +
> > +		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
> 
> You might want to add a "return" here to help the compiler (and the reader).

Done.

> > +	}
> > +
> > +	if (variant->expected_open_result == 0) {
> > +		fd = recv_fd(socket_fds[1]);
> > +		ASSERT_LE(0, fd);
> > +
> > +		EXPECT_EQ(variant->expected_ftruncate_result,
> > +			  test_ftruncate(fd));
> > +		ASSERT_EQ(0, close(fd));
> > +	}
> > +
> > +	ASSERT_EQ(child, waitpid(child, &status, 0));
> > +	ASSERT_EQ(1, WIFEXITED(status));
> > +	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> > +
> > +	ASSERT_EQ(0, close(socket_fds[0]));
> > +	ASSERT_EQ(0, close(socket_fds[1]));
> > +}
> > +
> >   /* clang-format off */
> >   FIXTURE(layout1_bind) {};
> >   /* clang-format on */

-- 

Re: [PATCH v8 8/9] samples/landlock: Extend sample tool to support LANDLOCK_ACCESS_FS_TRUNCATE

[Günther Noack]

On Wed, Oct 05, 2022 at 08:57:23PM +0200, Mickaël Salaün wrote:
> 
> On 01/10/2022 17:49, Günther Noack wrote:
> > 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>
> > ---
> >   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;
> 
> You can now base your patches on the current Linus' master branch, these
> three commits are now merged:
> https://git.kernel.org/mic/c/2fff00c81d4c37a037cf704d2d219fbcb45aea3c

Thanks, rebased.

> The (inlined) documentation also needs to be updated according to this
> commit to align with the double backtick convention.

There were no occurrences of the double backtick in the sample tool, I
assume this is OK?

> > +	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);

-- 

Re: [PATCH v8 9/9] landlock: Document Landlock's file truncation support

[Günther Noack]

On Wed, Oct 05, 2022 at 08:57:37PM +0200, Mickaël Salaün wrote:
> 
> On 01/10/2022 17:49, 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 | 66 +++++++++++++++++++++---
> >   1 file changed, 59 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index b8ea59493964..44d6f598b63d 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> > @@ -8,7 +8,7 @@ Landlock: unprivileged access control
> >   =====================================
> >   :Author: Mickaël Salaün
> > -:Date: May 2022
> > +:Date: October 2022
> >   The goal of Landlock is to enable to restrict ambient rights (e.g. global
> >   filesystem access) for a set of processes.  Because Landlock is a stackable
> > @@ -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,27 @@ 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`` or ``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) {
> > +    if (abi < 0) {
> > +        perror("The running kernel does not enable to use Landlock");
> 
> Please insert in a dedicated line this comment: /* Degrades gracefully if
> Landlock is not handled. */

Done, moved the comment to a dedicated line and added the "s".

> > +        return 0;  /* Degrade gracefully if Landlock is not handled. */
> > +    }
> > +    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;
> >       }
> >   This enables to create an inclusive ruleset that will contain our rules.
> > @@ -127,8 +139,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 +263,37 @@ 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 require the
> > +``LANDLOCK_ACCESS_FS_WRITE_FILE`` right.  Apart from the :manpage:`truncate(2)`
> > +system call, this can also be done through :manpage:`open(2)` with the flags
> > +``O_RDONLY | O_TRUNC``.
> > +
> > +When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
> > +right is associated with the newly created file descriptor and will be used for
> > +subsequent truncation attempts using :manpage:`ftruncate(2)`.  The behavior is
> > +similar to opening a file for reading or writing, where permissions are checked
> > +during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
> > +:manpage:`write(2)` calls.
> > +
> > +As a consequence, it is possible to have multiple open file descriptors for the
> > +same file, where one grants the right to truncate the file and the other does
> > +not.  It is also possible to pass such file descriptors between processes,
> > +keeping their Landlock properties, even when these processes do not have an
> > +enforced Landlock ruleset.
> > +
> >   Compatibility
> >   =============
> > @@ -397,6 +440,15 @@ Starting with the Landlock ABI version 2, it is now possible to securely
> >   control renaming and linking thanks to the new `LANDLOCK_ACCESS_FS_REFER`
> >   access right.
> > +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.
> > +
> >   .. _kernel_support:
> >   Kernel support

-- 

Re: [PATCH v8 0/9] landlock: truncate support

[Günther Noack]

On Wed, Oct 05, 2022 at 09:18:02PM +0200, Mickaël Salaün wrote:
> This patch series is almost ready, I guess the next one will be the final
> one.
> 
> I sent a related PR for syzkaller:
> https://github.com/google/syzkaller/pull/3423

Thank you for the review and the syzkaller patch! I sent out v9 just
now. The main fixes are the file_alloc_security hook, as well as tests
for it, and the various smaller formatting and indentation fixes. (See
cover letter changelog for the details.)

The crash reported by Nathan on the v8 patch set review was not
reproducable for me, even when following the exact repro steps and
compiling the kernel from scratch just to be sure.

—Günther

> On 01/10/2022 17:48, Günther Noack wrote:
> > The goal of these patches is to work towards a more complete coverage
> > of file system operations that are restrictable with Landlock.
> > 
> > Motivation
> > ----------
> > 
> > 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.
> > 
> > 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.
> > 
> > Implementation overview
> > -----------------------
> > 
> > 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.
> > 
> > Additionally, this patch set introduces a new Landlock security blob
> > associated with opened files, to track the available Landlock access
> > rights at the time of opening the file. This is in line with Unix's
> > general approach of checking the read and write permissions during
> > open(), and associating this previously checked authorization with the
> > opened file.
> > 
> > In order to treat truncate(2) and ftruncate(2) calls differently in an
> > LSM hook, we split apart the existing security_path_truncate hook into
> > security_path_truncate (for truncation by path) and
> > security_file_truncate (for truncation of previously opened files).
> > 
> > Relationship between "truncate" and "write" rights
> > --------------------------------------------------
> > 
> > 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.
> > 
> > When the "truncate" right is checked for ftruncate(2)
> > -----------------------------------------------------
> > 
> > Notably, for the purpose of ftruncate(2), the Landlock truncation
> > access right is looked up when *opening* the file, not when calling
> > ftruncate(). The availability of the truncate right is associated with
> > the opened file and is later checked to authorize ftruncate(2)
> > operations.
> > 
> > This is similar to how the write mode gets remembered after a
> > open(..., O_WRONLY) to authorize later write() operations.
> > 
> > These opened file descriptors can also be passed between processes and
> > will continue to enforce their truncation properties when these
> > processes attempt an ftruncate().
> > 
> > Ongoing discussions
> > -------------------
> > 
> > The one remaining ongoing discussion from v6 of the patch set is the
> > question whether we need to touch fs/ksmbd and fs/cachefiles, which
> > are both using vfs_truncate() to truncate files by path, even though
> > they already have the same struct file open. The proposal was to
> > introduce a "vfs_ftruncate()" that would work on opened files.
> > 
> > I think we should decouple this from the truncate patch set, with the
> > reasoning that:
> > 
> > (a) it would be a bigger change to create a "vfs_ftruncate()" which
> > would reach beyond the scope of this patch set.
> > 
> > (b) it seems likely that both components do not need to run under
> > Landlock at the moment and can be updated independently (just like it
> > needs to happen for normal userspace software in order to run it under
> > Landlock).
> > 
> > (c) vfs_truncate() is not the perfectly narrowest API for truncating
> > an opened file, but it's a legitimate way to do that and the operation
> > *is* checked with a Landlock LSM hook, although it might potentially
> > permit for a narrower sandboxing if that was done differently. That's
> > speculative though.
> > 
> > Overall, it's unclear whether doing this has any sandboxing benefits
> > for ksmbd and cachefiles, whereas on the downside, it would expand the
> > scope of the patch set quite a bit and would have to touch core parts
> > of the kernel (fs/open.c).
> > 
> > These patches are based on version 6.0-rc7.
> > 
> > Best regards,
> > Günther
> > 
> > [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> > 
> > Past discussions:
> > 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/
> > V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
> > V5: https://lore.kernel.org/all/20220817203006.21769-1-gnoack3000@gmail.com/
> > V6: https://lore.kernel.org/all/20220908195805.128252-1-gnoack3000@gmail.com/
> > V7: https://lore.kernel.org/all/20220930160144.141504-1-gnoack3000@gmail.com/
> > 
> > Changelog:
> > 
> > V8:
> > * landlock: Refactor check_access_path_dual() into
> >    is_access_to_paths_allowed(), as suggested by Mickaël Salaün on the
> >    v7 review. Added this as a separate commit.
> > * landlock truncate feature: inline get_path_access()
> > * Documentation: update documentation date to October 2022
> > * selftests: locally #define __maybe_unused (checkpatch started
> >    complaining about it, but the header where it's defined is not
> >    usable from selftests)
> > 
> > V7:
> > * security: Create file_truncate hook
> >    * Fix the build on kernels without CONFIG_SECURITY_PATH (fixed by
> >      Mickaël Salaün)
> >    * lsm_hooks.h: Document file_truncate hook
> >    * fs/open.c: undo accidental indentation changes
> > * landlock: Support file truncation
> >    * Use the init_layer_masks() result as input for
> >      check_access_path_dual()
> >    * Naming
> >      * Rename the landlock_file_security.allowed_access field
> >        (previously called "rights")
> >      * Rename get_path_access_rights() to get_path_access()
> >      * Rename get_file_access() to get_required_file_open_access() to
> >        avoid confusion with get_path_access()
> >      * Use "access_request" for access_mask_t variables, access_req for
> >        unsigned long
> >    * Documentation:
> >      * Fixed some comments according to review
> >      * Added comments to get_required_file_open_access() and
> >        init_layer_masks()
> > * selftests:
> >    * remove unused variables
> >    * rename fd0,...,fd3 to fd_layer0,...,fd_layer3.
> >    * test_ftruncate: define layers on top and inline the helper function
> > * New tests (both added as separate commits)
> >    * More exhaustive ftruncate test: Add open_and_ftruncate test that
> >      exercises ftruncate more thoroughly with fixture variants
> >    * FD-passing test: exercise restricted file descriptors getting
> >      passed between processes, also using the same fixture variants
> > * Documentation: integrate review comments by Mickaël Salaün
> >    * do not use contraptions (don't)
> >    * use double backquotes in all touched lines
> >    * add the read/write open() analogy to the truncation docs
> >    * in code example, check for abi<0 explicitly and fix indentation
> > 
> > V6:
> > * LSM hooks: create file_truncate hook in addition to path_truncate.
> >    Use it in the existing path_truncate call sites where appropriate.
> > * landlock: check LANDLOCK_ACCESS_FS_TRUNCATE right during open(), and
> >    associate that right with the opened struct file in a security blob.
> >    Introduce get_path_access_rights() helper function.
> > * selftests: test ftruncate in a separate test, to exercise that
> >    the rights are associated with the file descriptor.
> > * Documentation: Rework documentation to reflect new ftruncate() semantics.
> > * Applied small fixes by Mickaël Salaün which he added on top of V5, in
> >    https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> >    (I hope I found them all.)
> > 
> > V5:
> > * Documentation
> >    * Fix wording in userspace-api headers and in landlock.rst.
> >    * Move the truncation limitation section one to the bottom.
> >    * Move all .rst changes into the documentation commit.
> > * selftests
> >    * Remove _metadata argument from helpers where it became unnecessary.
> >    * Open writable file descriptors at the top of both tests, before Landlock
> >      is enabled, to exercise ftruncate() independently from open().
> >    * Simplify test_ftruncate and decouple it from exercising open().
> >    * test_creat(): Return errno on close() failure (it does not conflict).
> >    * Fix /* comment style */
> >    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
> >    * Add missing |O_TRUNC to a check in one test.
> >    * Put the truncate_unhandled test before the other.
> > 
> > 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.
> > 
> > Günther Noack (9):
> >    security: Create file_truncate hook from path_truncate hook
> >    selftests/landlock: Locally define __maybe_unused
> >    landlock: Refactor check_access_path_dual() into
> >      is_access_to_paths_allowed()
> >    landlock: Support file truncation
> >    selftests/landlock: Selftests for file truncation support
> >    selftests/landlock: Test open() and ftruncate() in multiple scenarios
> >    selftests/landlock: Test FD passing from a Landlock-restricted to an
> >      unrestricted process
> >    samples/landlock: Extend sample tool to support
> >      LANDLOCK_ACCESS_FS_TRUNCATE
> >    landlock: Document Landlock's file truncation support
> > 
> >   Documentation/userspace-api/landlock.rst     |  66 ++-
> >   fs/namei.c                                   |   2 +-
> >   fs/open.c                                    |   2 +-
> >   include/linux/lsm_hook_defs.h                |   1 +
> >   include/linux/lsm_hooks.h                    |  10 +-
> >   include/linux/security.h                     |   6 +
> >   include/uapi/linux/landlock.h                |  21 +-
> >   samples/landlock/sandboxer.c                 |  23 +-
> >   security/apparmor/lsm.c                      |   6 +
> >   security/landlock/fs.c                       | 191 +++++---
> >   security/landlock/fs.h                       |  24 +
> >   security/landlock/limits.h                   |   2 +-
> >   security/landlock/setup.c                    |   1 +
> >   security/landlock/syscalls.c                 |   2 +-
> >   security/security.c                          |   5 +
> >   security/tomoyo/tomoyo.c                     |  13 +
> >   tools/testing/selftests/landlock/base_test.c |  38 +-
> >   tools/testing/selftests/landlock/common.h    |  85 +++-
> >   tools/testing/selftests/landlock/fs_test.c   | 452 ++++++++++++++++++-
> >   19 files changed, 828 insertions(+), 122 deletions(-)
> > 
> > 
> > base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff

--