On Sat, May 31, 2025 at 02:51:22PM +0100, Tingmao Wang wrote:

On 5/28/25 23:26, Song Liu wrote:

quoted

Use path_parent() to walk a path up to its parent.

While path_parent() has an extra check with path_connected() than existing
code, there is no functional changes intended for landlock.

Signed-off-by: Song Liu <song@kernel.org>
---
 security/landlock/fs.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 6fee7c20f64d..32a24758ad6e 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c

@@ -837,7 +837,6 @@ static bool is_access_to_paths_allowed(
 	 * restriction.
 	 */
 	while (true) {
-		struct dentry *parent_dentry;
 		const struct landlock_rule *rule;
 
 		/*

@@ -896,19 +895,17 @@ static bool is_access_to_paths_allowed(
 		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))) {
+		switch (path_parent(&walker_path)) {
+		case PATH_PARENT_CHANGED_MOUNT:
+			/* Ignores hidden mount points. */
+			goto jump_up;
+		case PATH_PARENT_REAL_ROOT:
+			/*
+			 * Stops at the real root.  Denies access
+			 * because not all layers have granted access.
+			 */
+			goto walk_done;
+		case PATH_PARENT_DISCONNECTED_ROOT:
 			/*
 			 * Stops at disconnected root directories.  Only allows
 			 * access to internal filesystems (e.g. nsfs, which is

I was looking at the existing handling of disconnected root in Landlock
and I realized that the comment here confused me a bit:

/*
 * Stops at disconnected root directories.  Only allows
 * access to internal filesystems (e.g. nsfs, which is
 * reachable through /proc/<pid>/ns/<namespace>).
 */

In the original code, this was under a

    if (unlikely(IS_ROOT(walker_path.dentry)))

which means that it only stops walking if we found out we're disconnected
after reaching a filesystem boundary.  However if before we got to this
point, we have already collected enough rules to allow access, access
would be allowed, even if we're currently disconnected.  Demo:

/ # cd /
/ # cp /linux/samples/landlock/sandboxer .
/ # mkdir a b
/ # mkdir a/foo
/ # echo baz > a/foo/bar
/ # mount --bind a b
/ # LL_FS_RO=/ LL_FS_RW=/ ./sandboxer bash
Executing the sandboxed command...
/ # cd /b/foo
/b/foo # cat bar
baz
/b/foo # mv /a/foo /foo
/b/foo # cd ..     # <- We're now disconnected
bash: cd: ..: No such file or directory
/b/foo # cat bar
baz                # <- but landlock still lets us read the file

However, I think this patch will change this behavior due to the use of
path_connected

root@10a8fff999ce:/# mkdir a b
root@10a8fff999ce:/# mkdir a/foo
root@10a8fff999ce:/# echo baz > a/foo/bar
root@10a8fff999ce:/# mount --bind a b
root@10a8fff999ce:/# LL_FS_RO=/ LL_FS_RW=/ ./sandboxer bash
Executing the sandboxed command...
bash: cannot set terminal process group (191): Inappropriate ioctl for device
bash: no job control in this shell
root@10a8fff999ce:/# cd /b/foo
root@10a8fff999ce:/b/foo# cat bar
baz
root@10a8fff999ce:/b/foo# mv /a/foo /foo
root@10a8fff999ce:/b/foo# cd ..
bash: cd: ..: No such file or directory
root@10a8fff999ce:/b/foo# cat bar
cat: bar: Permission denied

This is a good test case, we should add a test for that.

I'm not sure if the original behavior was intentional, but since this
technically counts as a functional changes, just pointing this out.

This is indeed an issue.

Also I'm slightly worried about the performance overhead of doing
path_connected for every hop in the iteration (but ultimately it's
Mickaël's call).

Yes, we need to check with a benchmark.  We might want to keep the
walker_path.dentry == walker_path.mnt->mnt_root check inlined.

At least for Landlock, I think if we want to block all
access to disconnected files, as long as we eventually realize we have
been disconnected (by doing the "if dentry == path.mnt" check once when we
reach root), and in that case deny access, we should be good.

quoted

@@ -918,12 +915,15 @@ static bool is_access_to_paths_allowed(
 				allowed_parent1 = true;
 				allowed_parent2 = true;
 			}
+			goto walk_done;
+		case PATH_PARENT_SAME_MOUNT:
 			break;
+		default:
+			WARN_ON_ONCE(1);
+			goto walk_done;
 		}
-		parent_dentry = dget_parent(walker_path.dentry);
-		dput(walker_path.dentry);
-		walker_path.dentry = parent_dentry;
 	}
+walk_done:
 	path_put(&walker_path);
 
 	if (!allowed_parent1) {