[PATCH 0/2] landlock: Simplify path walk logic

[PATCH 0/2] landlock: Simplify path walk logic

[Justin Suess]

Hello,

These two patches simplify the path walk logic in fs.c.

This patch was originally included in a very basic form in my
LANDLOCK_ADD_RULE_NO_INHERIT series [1], but I think that it would be better
submitted separately, as logically it doesn't have much to do with the
feature implemented in the patch.

This patch is based on the mic/next branch.

Motivation
===

Additionally, existing path walk logic is tightly bound to the
is_access_to_paths_allowed and collect_domain_accesses, and is difficult to
read and understand.

Centralizing the path logic would more easily allow other Landlock features
that may rely on path walking, such as the proposed path walk controls, or
my LANDLOCK_ADD_RULE_NO_INHERIT patch, to reuse the same logic as
currently implemented.

Background
===

The first patch in this small series introduces a helper function
landlock_walk_path_up, which takes a pointer to a struct path, and walks it 
up through the VFS. The function returns an enum landlock_walk_result which
encodes whether the current path position is an internal mountpoint, the real
root, or neither.

The is_access_to_paths_allowed function is then altered to use this new helper,
cleaning up the traversal logic while retaining existing documentation comments
and improving readability.

The next patch in the series removes the collect_domain_accesses function. After
an initial re-implementation with the helper it was found that collect_domain_accesses
could be more succicently inlined into current_check_refer_path and there was little
benefit to keeping check_domain_accesses as a standalone function.

These changes overall reduce about 25 lines of code, including new documentation
for the return values of the landlock_walk_path_up function.

Results
===
These patches pass all existing selftests and kunit tests, and favorably influence
stack size.

Checkstack Results (CONFIG_AUDIT enabled)
===

Current Master Branch:
0xffffffff817d3f40 current_check_refer_path [vmlinux]:	608
0xffffffff817d2f80 is_access_to_paths_allowed [vmlinux]:352

This Patch Series:
0xffffffff817d3db0 current_check_refer_path [vmlinux]:	384
0xffffffff817d30c0 is_access_to_paths_allowed [vmlinux]:336

Thank you for your time.

Kind Regards,
Justin Suess

[1]: https://lore.kernel.org/linux-security-module/20251221194301.247484-2-utilityemal77@gmail.com/

Justin Suess (2):
  landlock: Add path walk helper
  landlock: Remove collect_domain_accesses

 security/landlock/fs.c | 220 ++++++++++++++++++-----------------------
 1 file changed, 98 insertions(+), 122 deletions(-)

-- 
2.51.0

[PATCH 1/2] landlock: Add path walk helper

[Justin Suess]

Add a new helper function landlock_walk_path_up, which takes a pointer
to the current path in the walk, and returns an enum
landlock_walk_result corresponding to whether the current position in
the walk is a mountpoint, the real root, or neither.

Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 security/landlock/fs.c | 92 ++++++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 40 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index e764470f588c..c6ff686c9cde 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -317,6 +317,38 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_IOCTL_DEV)
 /* clang-format on */
 
+/**
+ * enum landlock_walk_result - Result codes for landlock_walk_path_up()
+ * @LANDLOCK_WALK_CONTINUE: Path is now neither the real root nor an internal mount point.
+ * @LANDLOCK_WALK_STOP_REAL_ROOT: Path has reached the real VFS root.
+ * @LANDLOCK_WALK_INTERNAL: Path has reached an internal mount point.
+ */
+enum landlock_walk_result {
+	LANDLOCK_WALK_CONTINUE,
+	LANDLOCK_WALK_STOP_REAL_ROOT,
+	LANDLOCK_WALK_INTERNAL,
+};
+
+static enum landlock_walk_result landlock_walk_path_up(struct path *const path)
+{
+	struct dentry *old;
+
+	while (path->dentry == path->mnt->mnt_root) {
+		if (!follow_up(path))
+			return LANDLOCK_WALK_STOP_REAL_ROOT;
+	}
+	old = path->dentry;
+	if (unlikely(IS_ROOT(old))) {
+		if (likely(path->mnt->mnt_flags & MNT_INTERNAL))
+			return LANDLOCK_WALK_INTERNAL;
+		path->dentry = dget(path->mnt->mnt_root);
+	} else {
+		path->dentry = dget_parent(old);
+	}
+	dput(old);
+	return LANDLOCK_WALK_CONTINUE;
+}
+
 /*
  * @path: Should have been checked by get_path_from_fd().
  */
@@ -874,47 +906,27 @@ is_access_to_paths_allowed(const struct landlock_ruleset *const domain,
 		/* Stops when a rule from each layer grants access. */
 		if (allowed_parent1 && allowed_parent2)
 			break;
-
-jump_up:
-		if (walker_path.dentry == walker_path.mnt->mnt_root) {
-			if (follow_up(&walker_path)) {
-				/* Ignores hidden mount points. */
-				goto jump_up;
-			} else {
-				/*
-				 * Stops at the real root.  Denies access
-				 * because not all layers have granted access.
-				 */
-				break;
-			}
-		}
-
-		if (unlikely(IS_ROOT(walker_path.dentry))) {
-			if (likely(walker_path.mnt->mnt_flags & MNT_INTERNAL)) {
-				/*
-				 * Stops and allows access when reaching disconnected root
-				 * directories that are part of internal filesystems (e.g. nsfs,
-				 * which is reachable through /proc/<pid>/ns/<namespace>).
-				 */
-				allowed_parent1 = true;
-				allowed_parent2 = true;
-				break;
-			}
-
-			/*
-			 * We reached a disconnected root directory from a bind mount.
-			 * Let's continue the walk with the mount point we missed.
-			 */
-			dput(walker_path.dentry);
-			walker_path.dentry = walker_path.mnt->mnt_root;
-			dget(walker_path.dentry);
-		} else {
-			struct dentry *const parent_dentry =
-				dget_parent(walker_path.dentry);
-
-			dput(walker_path.dentry);
-			walker_path.dentry = parent_dentry;
+		/* Otherwise, keep walking up to the root. */
+		switch (landlock_walk_path_up(&walker_path)) {
+		/*
+		 * Stops and allows access when reaching disconnected root
+		 * directories that are part of internal filesystems (e.g. nsfs,
+		 * which is reachable through /proc/<pid>/ns/<namespace>).
+		 */
+		case LANDLOCK_WALK_INTERNAL:
+			allowed_parent1 = true;
+			allowed_parent2 = true;
+			break;
+		/*
+		 * Stops at the real root.  Denies access
+		 * because not all layers have granted access
+		 */
+		case LANDLOCK_WALK_STOP_REAL_ROOT:
+			break;
+		case LANDLOCK_WALK_CONTINUE:
+			continue;
 		}
+		break;
 	}
 	path_put(&walker_path);
 
-- 
2.51.0

[PATCH 2/2] landlock: Remove collect_domain_accesses

[Justin Suess]

Remove collect_domain_accesses and replace with inline logic using the
new path walk helper in the check_current_refer_path.

Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 security/landlock/fs.c | 128 +++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 82 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index c6ff686c9cde..efc65dc41c0d 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1013,77 +1013,6 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)
 				  LANDLOCK_ACCESS_FS_REMOVE_FILE;
 }
 
-/**
- * collect_domain_accesses - Walk through a file path and collect accesses
- *
- * @domain: Domain to check against.
- * @mnt_root: Last directory to check.
- * @dir: Directory to start the walk from.
- * @layer_masks_dom: Where to store the collected accesses.
- *
- * This helper is useful to begin a path walk from the @dir directory to a
- * @mnt_root directory used as a mount point.  This mount point is the common
- * ancestor between the source and the destination of a renamed and linked
- * file.  While walking from @dir to @mnt_root, we record all the domain's
- * allowed accesses in @layer_masks_dom.
- *
- * Because of disconnected directories, this walk may not reach @mnt_dir.  In
- * this case, the walk will continue to @mnt_dir after this call.
- *
- * 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;
- * - false if the walk reached @mnt_root.
- */
-static bool collect_domain_accesses(const struct landlock_ruleset *const domain,
-				    const struct dentry *const mnt_root,
-				    struct dentry *dir,
-				    struct layer_access_masks *layer_masks_dom)
-{
-	bool ret = false;
-
-	if (WARN_ON_ONCE(!domain || !mnt_root || !dir || !layer_masks_dom))
-		return true;
-	if (is_nouser_or_private(dir))
-		return true;
-
-	if (!landlock_init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
-				       layer_masks_dom, LANDLOCK_KEY_INODE))
-		return true;
-
-	dget(dir);
-	while (true) {
-		struct dentry *parent_dentry;
-
-		/* Gets all layers allowing all domain accesses. */
-		if (landlock_unmask_layers(find_rule(domain, dir),
-					   layer_masks_dom)) {
-			/*
-			 * Stops when all handled accesses are allowed by at
-			 * least one rule in each layer.
-			 */
-			ret = true;
-			break;
-		}
-
-		/*
-		 * Stops at the mount point or the filesystem root for a disconnected
-		 * directory.
-		 */
-		if (dir == mnt_root || unlikely(IS_ROOT(dir)))
-			break;
-
-		parent_dentry = dget_parent(dir);
-		dput(dir);
-		dir = parent_dentry;
-	}
-	dput(dir);
-	return ret;
-}
-
 /**
  * current_check_refer_path - Check if a rename or link action is allowed
  *
@@ -1147,7 +1076,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	bool allow_parent1, allow_parent2;
 	access_mask_t access_request_parent1, access_request_parent2;
 	struct path mnt_dir;
-	struct dentry *old_parent;
+	struct path old_parent_path;
 	struct layer_access_masks layer_masks_parent1 = {},
 				  layer_masks_parent2 = {};
 	struct landlock_request request1 = {}, request2 = {};
@@ -1202,20 +1131,55 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	/*
 	 * old_dentry may be the root of the common mount point and
 	 * !IS_ROOT(old_dentry) at the same time (e.g. with open_tree() and
-	 * OPEN_TREE_CLONE).  We do not need to call dget(old_parent) because
+	 * OPEN_TREE_CLONE).  We do not need to path_get(old_parent_path) because
 	 * we keep a reference to old_dentry.
 	 */
-	old_parent = (old_dentry == mnt_dir.dentry) ? old_dentry :
-						      old_dentry->d_parent;
+	old_parent_path.mnt = mnt_dir.mnt;
+	old_parent_path.dentry = unlikely(old_dentry == mnt_dir.dentry) ?
+					 old_dentry :
+					 old_dentry->d_parent;
 
 	/* new_dir->dentry is equal to new_dentry->d_parent */
-	allow_parent1 = collect_domain_accesses(subject->domain, mnt_dir.dentry,
-						old_parent,
-						&layer_masks_parent1);
-	allow_parent2 = collect_domain_accesses(subject->domain, mnt_dir.dentry,
-						new_dir->dentry,
-						&layer_masks_parent2);
+	allow_parent1 = false;
+	allow_parent2 = false;
+	for (size_t i = 0; i < 2; i++) {
+		const struct path *const parent_path = i ? new_dir :
+							   &old_parent_path;
+		struct layer_access_masks *const layer_masks =
+			i ? &layer_masks_parent2 : &layer_masks_parent1;
+		bool *const allow_parent = i ? &allow_parent2 : &allow_parent1;
+
+		if (is_nouser_or_private(parent_path->dentry) ||
+		    !landlock_init_layer_masks(
+			    subject->domain, LANDLOCK_MASK_ACCESS_FS,
+			    layer_masks, LANDLOCK_KEY_INODE)) {
+			*allow_parent = true;
+			continue;
+		}
 
+		{
+			struct path walker = *parent_path;
+
+			path_get(&walker);
+			do {
+				/* Gets all layers allowing all domain accesses. */
+				if (landlock_unmask_layers(
+					    find_rule(subject->domain,
+						      walker.dentry),
+					    layer_masks)) {
+					/*
+					 * Stops when all handled accesses are
+					 * allowed by at least one rule in each
+					 * layer.
+					 */
+					*allow_parent = true;
+					break;
+				}
+			} while (landlock_walk_path_up(&walker) ==
+				 LANDLOCK_WALK_CONTINUE);
+			path_put(&walker);
+		}
+	}
 	if (allow_parent1 && allow_parent2)
 		return 0;
 
@@ -1233,7 +1197,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 		return 0;
 
 	if (request1.access) {
-		request1.audit.u.path.dentry = old_parent;
+		request1.audit.u.path.dentry = old_parent_path.dentry;
 		landlock_log_denial(subject, &request1);
 	}
 	if (request2.access) {
-- 
2.51.0