[PATCH] selftests/landlock: Add tests for access through disconnected paths

[PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

This adds a test for the edge case discussed in [1], and in addition also
test rename operations when the operands are through disconnected paths,
as that go through a separate code path in Landlock.

[1]: https://lore.kernel.org/linux-security-module/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org/

This has resulted in a WARNING, due to collect_domain_accesses() not
expecting to reach a different root from path->mnt:

	#  RUN           layout1_bind.path_disconnected ...
	#            OK  layout1_bind.path_disconnected
	ok 96 layout1_bind.path_disconnected
	#  RUN           layout1_bind.path_disconnected_rename ...
	[..] ------------[ cut here ]------------
	[..] WARNING: CPU: 3 PID: 385 at security/landlock/fs.c:1065 collect_domain_accesses
	[..] ...
	[..] RIP: 0010:collect_domain_accesses (security/landlock/fs.c:1065 (discriminator 2) security/landlock/fs.c:1031 (discriminator 2))
	[..] current_check_refer_path (security/landlock/fs.c:1205)
	[..] ...
	[..] hook_path_rename (security/landlock/fs.c:1526)
	[..] security_path_rename (security/security.c:2026 (discriminator 1))
	[..] do_renameat2 (fs/namei.c:5264)
	#            OK  layout1_bind.path_disconnected_rename
	ok 97 layout1_bind.path_disconnected_rename

My understanding is that terminating at the mountpoint is basically an
optimization, so that for rename operations we only walks the path from
the mountpoint to the real root once.  We probably want to keep this
optimization, as disconnected paths are probably a very rare edge case.

This might need more thinking, but maybe if one of the operands is
disconnected, we can just let it walk until IS_ROOT(dentry), and also
collect access for the other path until IS_ROOT(dentry), then call
is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
this case is_access_to_paths_allowed will not do any walking and just make
an access decision.)

Letting the walk continue until IS_ROOT(dentry) is what
is_access_to_paths_allowed() effectively does for non-renames.

(Also note: moving the const char definitions a bit above so that we can
use the path for s4d1 in cleanup code.)

Signed-off-by: Tingmao Wang <m@maowtm.org>
---
 tools/testing/selftests/landlock/fs_test.c | 271 ++++++++++++++++++++-
 1 file changed, 268 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 73729382d40f..d042a742a1c5 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4521,6 +4521,17 @@ TEST_F_FORK(ioctl, handle_file_access_file)
 FIXTURE(layout1_bind) {};
 /* clang-format on */
 
+static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
+static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
+/* Move targets for disconnected path tests */
+static const char dir_s4d1[] = TMP_DIR "/s4d1";
+static const char file1_s4d1[] = TMP_DIR "/s4d1/f1";
+static const char file2_s4d1[] = TMP_DIR "/s4d1/f2";
+static const char dir_s4d2[] = TMP_DIR "/s4d1/s4d2";
+static const char file1_s4d2[] = TMP_DIR "/s4d1/s4d2/f1";
+static const char file1_name[] = "f1";
+static const char file2_name[] = "f2";
+
 FIXTURE_SETUP(layout1_bind)
 {
 	prepare_layout(_metadata);
@@ -4536,14 +4547,14 @@ FIXTURE_TEARDOWN_PARENT(layout1_bind)
 {
 	/* umount(dir_s2d2)) is handled by namespace lifetime. */
 
+	remove_path(file1_s4d1);
+	remove_path(file2_s4d1);
+
 	remove_layout1(_metadata);
 
 	cleanup_layout(_metadata);
 }
 
-static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
-static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
-
 /*
  * layout1_bind hierarchy:
  *
@@ -4766,6 +4777,260 @@ TEST_F_FORK(layout1_bind, reparent_cross_mount)
 	ASSERT_EQ(0, rename(bind_file1_s1d3, file1_s2d2));
 }
 
+/*
+ * Make sure access to file through a disconnected path works as expected.
+ * This test uses s4d1 as the move target.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected)
+{
+	const struct rule layer1_allow_all[] = {
+		{
+			.path = TMP_DIR,
+			.access = ACCESS_ALL,
+		},
+		{},
+	};
+
+	const struct rule layer2_allow_just_f1[] = {
+		{
+			.path = file1_s1d3,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{},
+	};
+
+	const struct rule layer3_only_s1d2[] = {
+		{
+			.path = dir_s1d2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{},
+	};
+
+	/* Landlock should not deny access just because it is disconnected */
+	int ruleset_fd =
+		create_ruleset(_metadata, ACCESS_ALL, layer1_allow_all);
+	/*
+	 * Create the new ruleset now before we move the dir containing the
+	 * file
+	 */
+	int ruleset_fd_l2 =
+		create_ruleset(_metadata, ACCESS_RW, layer2_allow_just_f1);
+	int ruleset_fd_l3 =
+		create_ruleset(_metadata, ACCESS_RW, layer3_only_s1d2);
+	int bind_s1d3_fd;
+
+	ASSERT_LE(0, ruleset_fd);
+	ASSERT_LE(0, ruleset_fd_l2);
+	ASSERT_LE(0, ruleset_fd_l3);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+
+	ASSERT_LE(0, bind_s1d3_fd);
+	/* Test access is possible before we move */
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+	/* Make it disconnected */
+	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
+	{
+		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
+		       strerror(errno));
+	}
+	/* Test access still possible */
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+	/*
+	 * Test ".." not possibe (not because of landlock, but just because
+	 * it's disconnected)
+	 */
+	ASSERT_EQ(ENOENT,
+		  test_open_rel(bind_s1d3_fd, "..", O_RDONLY | O_DIRECTORY));
+
+	/* Should still work with a narrower rule */
+	enforce_ruleset(_metadata, ruleset_fd_l2);
+	ASSERT_EQ(0, close(ruleset_fd_l2));
+
+	ASSERT_EQ(0, test_open(file1_s4d1, O_RDONLY));
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+	ASSERT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+
+	/*
+	 * But if we only allow access to under the original dir, then it
+	 * should be denied.
+	 */
+	enforce_ruleset(_metadata, ruleset_fd_l3);
+	ASSERT_EQ(0, close(ruleset_fd_l3));
+	ASSERT_EQ(EACCES, test_open(file1_s4d1, O_RDONLY));
+	ASSERT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+}
+
+/*
+ * Test that we can rename to make files disconnected, and rename it back,
+ * under landlock.  This test uses s4d2 as the move target, so that we can
+ * have a rule allowing refers on the move target's immediate parent.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected_rename)
+{
+	const struct rule layer1[] = {
+		{
+			.path = dir_s1d2,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MAKE_DIR |
+				  LANDLOCK_ACCESS_FS_REMOVE_DIR |
+				  LANDLOCK_ACCESS_FS_MAKE_REG |
+				  LANDLOCK_ACCESS_FS_REMOVE_FILE |
+				  LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{
+			.path = dir_s4d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MAKE_DIR |
+				  LANDLOCK_ACCESS_FS_REMOVE_DIR |
+				  LANDLOCK_ACCESS_FS_MAKE_REG |
+				  LANDLOCK_ACCESS_FS_REMOVE_FILE |
+				  LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{}
+	};
+
+	const struct rule layer2_only_s1d2[] = {
+		{
+			.path = dir_s1d2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{},
+	};
+
+	ASSERT_EQ(0, mkdir(dir_s4d1, 0755))
+	{
+		TH_LOG("Failed to create %s: %s", dir_s4d1, strerror(errno));
+	}
+
+	int ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, layer1);
+	int ruleset_fd_l2 = create_ruleset(
+		_metadata, LANDLOCK_ACCESS_FS_READ_FILE, layer2_only_s1d2);
+	pid_t child_pid;
+	int bind_s1d3_fd, status;
+
+	ASSERT_LE(0, ruleset_fd);
+	ASSERT_LE(0, ruleset_fd_l2);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+	ASSERT_LE(0, bind_s1d3_fd);
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+	{
+		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+		       strerror(errno));
+	}
+
+	/*
+	 * Since file is no longer under s1d2, we should not be able to access
+	 * it if we enforced layer 2.  Do a fork to test this so we don't
+	 * prevent ourselves from renaming it back later.
+	 */
+	child_pid = fork();
+	if (child_pid == 0) {
+		enforce_ruleset(_metadata, ruleset_fd_l2);
+		ASSERT_EQ(0, close(ruleset_fd_l2));
+		ASSERT_EQ(EACCES,
+			  test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+		ASSERT_EQ(EACCES, test_open(file1_s4d2, O_RDONLY));
+
+		/*
+		 * Test that access widening checks indeed prevents us from
+		 * renaming it back
+		 */
+		ASSERT_EQ(-1, rename(dir_s4d2, dir_s1d3));
+		ASSERT_EQ(EXDEV, errno);
+		/*
+		 * Including through the now disconnected fd (but it should return
+		 * EXDEV)
+		 */
+		ASSERT_EQ(-1, renameat(bind_s1d3_fd, file1_name, AT_FDCWD,
+				       file1_s2d2));
+		ASSERT_EQ(EXDEV, errno);
+		_exit(!__test_passed(_metadata));
+		return;
+	}
+
+	ASSERT_NE(-1, child_pid);
+	ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0));
+	ASSERT_EQ(1, WIFEXITED(status));
+	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+	ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
+	{
+		TH_LOG("Failed to rename %s back to %s: %s", dir_s4d1, dir_s1d3,
+		       strerror(errno));
+	}
+
+	/* Now check that we can access it under l2 */
+	child_pid = fork();
+	if (child_pid == 0) {
+		enforce_ruleset(_metadata, ruleset_fd_l2);
+		ASSERT_EQ(0, close(ruleset_fd_l2));
+		ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+		ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+		_exit(!__test_passed(_metadata));
+		return;
+	}
+	ASSERT_NE(-1, child_pid);
+	ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0));
+	ASSERT_EQ(1, WIFEXITED(status));
+	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+	/*
+	 * Also test that we can rename via a disconnected path.  We move the
+	 * dir back to the disconnected place first, then we rename file1 to
+	 * file2 through our dir fd.
+	 */
+	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+	{
+		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+		       strerror(errno));
+	}
+	ASSERT_EQ(0,
+		  renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name))
+	{
+		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+		       file1_name, file2_name, bind_dir_s1d3, strerror(errno));
+	}
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+	ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
+	{
+		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+		       file2_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
+	}
+	ASSERT_EQ(0, test_open(file1_s2d2, O_RDONLY));
+	ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY));
+
+	/* Move it back using the disconnected path as the target */
+	ASSERT_EQ(0, renameat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file1_name))
+	{
+		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+		       file1_s1d2, file1_name, bind_dir_s1d3, strerror(errno));
+	}
+
+	/* Now make it connected again */
+	ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
+	{
+		TH_LOG("Failed to rename %s back to %s: %s", dir_s4d2, dir_s1d3,
+		       strerror(errno));
+	}
+
+	/* Check again that we can access it under l2 */
+	enforce_ruleset(_metadata, ruleset_fd_l2);
+	ASSERT_EQ(0, close(ruleset_fd_l2));
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+	ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+}
+
 #define LOWER_BASE TMP_DIR "/lower"
 #define LOWER_DATA LOWER_BASE "/data"
 static const char lower_fl1[] = LOWER_DATA "/fl1";

base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.49.0

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

On 6/14/25 19:25, Tingmao Wang wrote:
> This adds a test for the edge case discussed in [1], and in addition also
> test rename operations when the operands are through disconnected paths,
> as that go through a separate code path in Landlock.
> [..]

Slightly improve comments a bit...

(Another edit to add test for linkat to follow)

---
 tools/testing/selftests/landlock/fs_test.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d042a742a1c5..53b167dbd39c 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4779,7 +4779,7 @@ TEST_F_FORK(layout1_bind, reparent_cross_mount)
 
 /*
  * Make sure access to file through a disconnected path works as expected.
- * This test uses s4d1 as the move target.
+ * This test moves s1d3 to s4d1.
  */
 TEST_F_FORK(layout1_bind, path_disconnected)
 {
@@ -4866,9 +4866,9 @@ TEST_F_FORK(layout1_bind, path_disconnected)
 }
 
 /*
- * Test that we can rename to make files disconnected, and rename it back,
- * under landlock.  This test uses s4d2 as the move target, so that we can
- * have a rule allowing refers on the move target's immediate parent.
+ * Test that renameat with disconnected paths works under landlock.  This
+ * test moves s1d3 to s4d2, so that we can have a rule allowing refers on
+ * the move target's immediate parent.
  */
 TEST_F_FORK(layout1_bind, path_disconnected_rename)
 {
@@ -4998,7 +4998,7 @@ TEST_F_FORK(layout1_bind, path_disconnected_rename)
 	ASSERT_EQ(0,
 		  renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name))
 	{
-		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+		TH_LOG("Failed to rename %s to %s within disconnected %s: %s",
 		       file1_name, file2_name, bind_dir_s1d3, strerror(errno));
 	}
 	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));

-- 
2.49.0

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

On 6/14/25 19:25, Tingmao Wang wrote:
> This adds a test for the edge case discussed in [1], and in addition also
> test rename operations when the operands are through disconnected paths,
> as that go through a separate code path in Landlock.
> [..]

Hi,

Since linkat also uses the separate refer check, and since it's not too
difficult given what we already have, I've made an edit to add tests for
linkat under disconnected paths as well.

Hopefully this helps better confirm that any fix we make does not change
existing user-facing behaviour (unless, of course, if we want to).

Tested with qemu as well as UML. Passes without panic_on_warn.

Signed-off-by: Tingmao Wang <m@maowtm.org>
---
 tools/testing/selftests/landlock/fs_test.c | 118 +++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 53b167dbd39c..055f6de25d05 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -5031,6 +5031,124 @@ TEST_F_FORK(layout1_bind, path_disconnected_rename)
 	ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
 }
 
+/*
+ * Test that linkat with disconnected paths works under Landlock. This
+ * test moves s1d3 to s4d1.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected_link)
+{
+	/* Ruleset to be applied after renaming s1d3 to s4d1 */
+	const struct rule layer1[] = {
+		{
+			.path = dir_s4d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_MAKE_REG |
+				  LANDLOCK_ACCESS_FS_REMOVE_FILE,
+		},
+		{
+			.path = dir_s2d2,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_MAKE_REG |
+				  LANDLOCK_ACCESS_FS_REMOVE_FILE,
+		},
+		{}
+	};
+	int ruleset_fd, bind_s1d3_fd;
+
+	/* Remove unneeded files created by layout1, otherwise we will EEXIST */
+	ASSERT_EQ(0, unlink(file1_s1d2));
+	ASSERT_EQ(0, unlink(file2_s1d3));
+
+	bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+	ASSERT_LE(0, bind_s1d3_fd);
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+	/* Make bind_s1d3_fd disconnected */
+	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
+	{
+		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
+		       strerror(errno));
+	}
+	/* Need this later to test different parent link */
+	ASSERT_EQ(0, mkdir(dir_s4d2, 0755))
+	{
+		TH_LOG("Failed to create %s: %s", dir_s4d2, strerror(errno));
+	}
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, layer1);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* disconnected to connected */
+	ASSERT_EQ(0, linkat(bind_s1d3_fd, file1_name, AT_FDCWD, file1_s2d2, 0))
+	{
+		TH_LOG("Failed to link %s to %s via disconnected %s: %s",
+		       file1_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
+	}
+	/* Test that we can access via the new link */
+	ASSERT_EQ(0, test_open(file1_s2d2, O_RDONLY))
+	{
+		TH_LOG("Failed to open newly linked %s: %s", file1_s2d2,
+		       strerror(errno));
+	}
+	/* As well as the old one */
+	ASSERT_EQ(0, test_open(file1_s4d1, O_RDONLY))
+	{
+		TH_LOG("Failed to open original %s: %s", file1_s4d1,
+		       strerror(errno));
+	}
+
+	/* connected to disconnected */
+	ASSERT_EQ(0, unlink(file1_s4d1));
+	ASSERT_EQ(0, linkat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file2_name, 0))
+	{
+		TH_LOG("Failed to link %s to %s via disconnected %s: %s",
+		       file1_s2d2, file2_name, bind_dir_s1d3, strerror(errno));
+	}
+	ASSERT_EQ(0, test_open(file2_s4d1, O_RDONLY));
+	ASSERT_EQ(0, unlink(file1_s2d2));
+
+	/* disconnected to disconnected (same parent) */
+	ASSERT_EQ(0,
+		  linkat(bind_s1d3_fd, file2_name, bind_s1d3_fd, file1_name, 0))
+	{
+		TH_LOG("Failed to link %s to %s within disconnected %s: %s",
+		       file2_name, file1_name, bind_dir_s1d3, strerror(errno));
+	}
+	ASSERT_EQ(0, test_open(file1_s4d1, O_RDONLY))
+	{
+		TH_LOG("Failed to open newly linked %s: %s", file1_s4d1,
+		       strerror(errno));
+	}
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY))
+	{
+		TH_LOG("Failed to open %s through newly created link under disconnected path: %s",
+		       file1_name, strerror(errno));
+	}
+	ASSERT_EQ(0, unlink(file2_s4d1));
+
+	/* disconnected to disconnected (different parent) */
+	ASSERT_EQ(0,
+		  linkat(bind_s1d3_fd, file1_name, bind_s1d3_fd, "s4d2/f1", 0))
+	{
+		TH_LOG("Failed to link %s to %s within disconnected %s: %s",
+		       file1_name, "s4d2/f1", bind_dir_s1d3, strerror(errno));
+	}
+	ASSERT_EQ(0, test_open(file1_s4d2, O_RDONLY))
+	{
+		TH_LOG("Failed to open %s after link: %s", file1_s4d2,
+		       strerror(errno));
+	}
+	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, "s4d2/f1", O_RDONLY))
+	{
+		TH_LOG("Failed to open %s through disconnected path after link: %s",
+		       "s4d2/f1", strerror(errno));
+	}
+}
+
 #define LOWER_BASE TMP_DIR "/lower"
 #define LOWER_DATA LOWER_BASE "/data"
 static const char lower_fl1[] = LOWER_DATA "/fl1";
-- 
2.49.0

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Mickaël Salaün]

On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:
> This adds a test for the edge case discussed in [1], and in addition also
> test rename operations when the operands are through disconnected paths,
> as that go through a separate code path in Landlock.
> 
> [1]: https://lore.kernel.org/linux-security-module/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org/
> 
> This has resulted in a WARNING, due to collect_domain_accesses() not
> expecting to reach a different root from path->mnt:
> 
> 	#  RUN           layout1_bind.path_disconnected ...
> 	#            OK  layout1_bind.path_disconnected
> 	ok 96 layout1_bind.path_disconnected
> 	#  RUN           layout1_bind.path_disconnected_rename ...
> 	[..] ------------[ cut here ]------------
> 	[..] WARNING: CPU: 3 PID: 385 at security/landlock/fs.c:1065 collect_domain_accesses
> 	[..] ...
> 	[..] RIP: 0010:collect_domain_accesses (security/landlock/fs.c:1065 (discriminator 2) security/landlock/fs.c:1031 (discriminator 2))
> 	[..] current_check_refer_path (security/landlock/fs.c:1205)
> 	[..] ...
> 	[..] hook_path_rename (security/landlock/fs.c:1526)
> 	[..] security_path_rename (security/security.c:2026 (discriminator 1))
> 	[..] do_renameat2 (fs/namei.c:5264)
> 	#            OK  layout1_bind.path_disconnected_rename
> 	ok 97 layout1_bind.path_disconnected_rename

Good catch and thanks for the tests!  I sent a fix:
https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/

> 
> My understanding is that terminating at the mountpoint is basically an
> optimization, so that for rename operations we only walks the path from
> the mountpoint to the real root once.  We probably want to keep this
> optimization, as disconnected paths are probably a very rare edge case.

Rename operations can only happen within the same mount point, otherwise
the kernel returns -EXDEV.  The collect_domain_accesses() is called for
the source and the destination of a rename to walk to their common mount
point, if any.  We could maybe improve this walk by doing them at the
same time but because we don't know the depth of each path, I'm not sure
the required extra complexity would be worth it.  The current approach
is simple and opportunistically limits the walks.

> 
> This might need more thinking, but maybe if one of the operands is
> disconnected, we can just let it walk until IS_ROOT(dentry), and also
> collect access for the other path until IS_ROOT(dentry), then call
> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
> this case is_access_to_paths_allowed will not do any walking and just make
> an access decision.)

If one side is in a disconnected directory and not the other side, the
rename would be denied by the VFS, but Landlock should still log (and
then deny) the side that would be denied anyway.

> 
> Letting the walk continue until IS_ROOT(dentry) is what
> is_access_to_paths_allowed() effectively does for non-renames.
> 
> (Also note: moving the const char definitions a bit above so that we can
> use the path for s4d1 in cleanup code.)
> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>

I squashed your patches and push them to my next branch with some minor
changes.  Please let me know if there is something wrong.

> ---
>  tools/testing/selftests/landlock/fs_test.c | 271 ++++++++++++++++++++-
>  1 file changed, 268 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 73729382d40f..d042a742a1c5 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4521,6 +4521,17 @@ TEST_F_FORK(ioctl, handle_file_access_file)
>  FIXTURE(layout1_bind) {};
>  /* clang-format on */
>  
> +static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
> +static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
> +/* Move targets for disconnected path tests */
> +static const char dir_s4d1[] = TMP_DIR "/s4d1";
> +static const char file1_s4d1[] = TMP_DIR "/s4d1/f1";
> +static const char file2_s4d1[] = TMP_DIR "/s4d1/f2";
> +static const char dir_s4d2[] = TMP_DIR "/s4d1/s4d2";
> +static const char file1_s4d2[] = TMP_DIR "/s4d1/s4d2/f1";
> +static const char file1_name[] = "f1";
> +static const char file2_name[] = "f2";
> +
>  FIXTURE_SETUP(layout1_bind)
>  {
>  	prepare_layout(_metadata);
> @@ -4536,14 +4547,14 @@ FIXTURE_TEARDOWN_PARENT(layout1_bind)
>  {
>  	/* umount(dir_s2d2)) is handled by namespace lifetime. */
>  
> +	remove_path(file1_s4d1);
> +	remove_path(file2_s4d1);
> +
>  	remove_layout1(_metadata);
>  
>  	cleanup_layout(_metadata);
>  }
>  
> -static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
> -static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
> -
>  /*
>   * layout1_bind hierarchy:
>   *
> @@ -4766,6 +4777,260 @@ TEST_F_FORK(layout1_bind, reparent_cross_mount)
>  	ASSERT_EQ(0, rename(bind_file1_s1d3, file1_s2d2));
>  }
>  
> +/*
> + * Make sure access to file through a disconnected path works as expected.
> + * This test uses s4d1 as the move target.
> + */
> +TEST_F_FORK(layout1_bind, path_disconnected)
> +{
> +	const struct rule layer1_allow_all[] = {
> +		{
> +			.path = TMP_DIR,
> +			.access = ACCESS_ALL,
> +		},
> +		{},
> +	};
> +
> +	const struct rule layer2_allow_just_f1[] = {
> +		{
> +			.path = file1_s1d3,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{},
> +	};
> +
> +	const struct rule layer3_only_s1d2[] = {
> +		{
> +			.path = dir_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{},
> +	};
> +
> +	/* Landlock should not deny access just because it is disconnected */
> +	int ruleset_fd =
> +		create_ruleset(_metadata, ACCESS_ALL, layer1_allow_all);
> +	/*
> +	 * Create the new ruleset now before we move the dir containing the
> +	 * file

Please use third person for most comments, and end them with a final dot
(for sentences).

> +	 */
> +	int ruleset_fd_l2 =
> +		create_ruleset(_metadata, ACCESS_RW, layer2_allow_just_f1);
> +	int ruleset_fd_l3 =
> +		create_ruleset(_metadata, ACCESS_RW, layer3_only_s1d2);
> +	int bind_s1d3_fd;
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	ASSERT_LE(0, ruleset_fd_l2);
> +	ASSERT_LE(0, ruleset_fd_l3);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));

I'll replace most ASSERT_* with EXPECT_*
ASSERT should only be used when checking a dependency for the following
tests.  It's not apply everywhere but new tests should follow this rule.

> +
> +	bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
> +
> +	ASSERT_LE(0, bind_s1d3_fd);
> +	/* Test access is possible before we move */
> +	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +	/* Make it disconnected */
> +	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
> +	{
> +		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
> +		       strerror(errno));
> +	}
> +	/* Test access still possible */
> +	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +	/*
> +	 * Test ".." not possibe (not because of landlock, but just because
> +	 * it's disconnected)
> +	 */
> +	ASSERT_EQ(ENOENT,
> +		  test_open_rel(bind_s1d3_fd, "..", O_RDONLY | O_DIRECTORY));
> +
> +	/* Should still work with a narrower rule */
> +	enforce_ruleset(_metadata, ruleset_fd_l2);
> +	ASSERT_EQ(0, close(ruleset_fd_l2));
> +
> +	ASSERT_EQ(0, test_open(file1_s4d1, O_RDONLY));
> +	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +	ASSERT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
> +
> +	/*
> +	 * But if we only allow access to under the original dir, then it
> +	 * should be denied.
> +	 */
> +	enforce_ruleset(_metadata, ruleset_fd_l3);
> +	ASSERT_EQ(0, close(ruleset_fd_l3));
> +	ASSERT_EQ(EACCES, test_open(file1_s4d1, O_RDONLY));
> +	ASSERT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +}
> +
> +/*
> + * Test that we can rename to make files disconnected, and rename it back,
> + * under landlock.  This test uses s4d2 as the move target, so that we can
> + * have a rule allowing refers on the move target's immediate parent.
> + */
> +TEST_F_FORK(layout1_bind, path_disconnected_rename)
> +{
> +	const struct rule layer1[] = {
> +		{
> +			.path = dir_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MAKE_DIR |
> +				  LANDLOCK_ACCESS_FS_REMOVE_DIR |
> +				  LANDLOCK_ACCESS_FS_MAKE_REG |
> +				  LANDLOCK_ACCESS_FS_REMOVE_FILE |
> +				  LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{
> +			.path = dir_s4d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MAKE_DIR |
> +				  LANDLOCK_ACCESS_FS_REMOVE_DIR |
> +				  LANDLOCK_ACCESS_FS_MAKE_REG |
> +				  LANDLOCK_ACCESS_FS_REMOVE_FILE |
> +				  LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{}
> +	};
> +
> +	const struct rule layer2_only_s1d2[] = {
> +		{
> +			.path = dir_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{},
> +	};
> +
> +	ASSERT_EQ(0, mkdir(dir_s4d1, 0755))
> +	{
> +		TH_LOG("Failed to create %s: %s", dir_s4d1, strerror(errno));
> +	}
> +
> +	int ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, layer1);
> +	int ruleset_fd_l2 = create_ruleset(
> +		_metadata, LANDLOCK_ACCESS_FS_READ_FILE, layer2_only_s1d2);
> +	pid_t child_pid;
> +	int bind_s1d3_fd, status;

I'll move variable declarations before mkdir call.

> +
> +	ASSERT_LE(0, ruleset_fd);
> +	ASSERT_LE(0, ruleset_fd_l2);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
> +	ASSERT_LE(0, bind_s1d3_fd);
> +	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +

I'll add these checks to test ENOENT priority over EACCES for
disconnected directory:

EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, "..", O_DIRECTORY));

> +	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
> +	{
> +		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
> +		       strerror(errno));
> +	}

EXPECT_EQ(ENOENT, test_open_rel(bind_s1d3_fd, "..", O_DIRECTORY));

> +
> +	/*
> +	 * Since file is no longer under s1d2, we should not be able to access
> +	 * it if we enforced layer 2.  Do a fork to test this so we don't
> +	 * prevent ourselves from renaming it back later.
> +	 */
> +	child_pid = fork();
> +	if (child_pid == 0) {
> +		enforce_ruleset(_metadata, ruleset_fd_l2);
> +		ASSERT_EQ(0, close(ruleset_fd_l2));
> +		ASSERT_EQ(EACCES,
> +			  test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +		ASSERT_EQ(EACCES, test_open(file1_s4d2, O_RDONLY));
> +
> +		/*
> +		 * Test that access widening checks indeed prevents us from
> +		 * renaming it back
> +		 */
> +		ASSERT_EQ(-1, rename(dir_s4d2, dir_s1d3));
> +		ASSERT_EQ(EXDEV, errno);
> +		/*
> +		 * Including through the now disconnected fd (but it should return
> +		 * EXDEV)
> +		 */
> +		ASSERT_EQ(-1, renameat(bind_s1d3_fd, file1_name, AT_FDCWD,
> +				       file1_s2d2));
> +		ASSERT_EQ(EXDEV, errno);
> +		_exit(!__test_passed(_metadata));

I prefer to use _exit(_metadata->exit_code) for the parent to get the
exit code (which would be printed with the ASSERT/EXPECT if unexpected)
and for consistency with other tests.  I'll change that.

> +		return;
> +	}
> +
> +	ASSERT_NE(-1, child_pid);
> +	ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0));
> +	ASSERT_EQ(1, WIFEXITED(status));
> +	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> +
> +	ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
> +	{
> +		TH_LOG("Failed to rename %s back to %s: %s", dir_s4d1, dir_s1d3,
> +		       strerror(errno));
> +	}
> +
> +	/* Now check that we can access it under l2 */
> +	child_pid = fork();
> +	if (child_pid == 0) {
> +		enforce_ruleset(_metadata, ruleset_fd_l2);
> +		ASSERT_EQ(0, close(ruleset_fd_l2));
> +		ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +		ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
> +		_exit(!__test_passed(_metadata));
> +		return;
> +	}
> +	ASSERT_NE(-1, child_pid);
> +	ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0));
> +	ASSERT_EQ(1, WIFEXITED(status));
> +	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> +
> +	/*
> +	 * Also test that we can rename via a disconnected path.  We move the
> +	 * dir back to the disconnected place first, then we rename file1 to
> +	 * file2 through our dir fd.
> +	 */
> +	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
> +	{
> +		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
> +		       strerror(errno));
> +	}
> +	ASSERT_EQ(0,
> +		  renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name))

Renaming files inside the same directory should always work.  The
following renameat(2) call checks with different directory, as well as
the link tests, so we are good.

> +	{
> +		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
> +		       file1_name, file2_name, bind_dir_s1d3, strerror(errno));
> +	}
> +	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
> +	ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
> +	{
> +		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
> +		       file2_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
> +	}
> +	ASSERT_EQ(0, test_open(file1_s2d2, O_RDONLY));
> +	ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY));
> +
> +	/* Move it back using the disconnected path as the target */
> +	ASSERT_EQ(0, renameat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file1_name))
> +	{
> +		TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
> +		       file1_s1d2, file1_name, bind_dir_s1d3, strerror(errno));
> +	}
> +
> +	/* Now make it connected again */
> +	ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
> +	{
> +		TH_LOG("Failed to rename %s back to %s: %s", dir_s4d2, dir_s1d3,
> +		       strerror(errno));
> +	}
> +
> +	/* Check again that we can access it under l2 */
> +	enforce_ruleset(_metadata, ruleset_fd_l2);
> +	ASSERT_EQ(0, close(ruleset_fd_l2));
> +	ASSERT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
> +	ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
> +}
> +
>  #define LOWER_BASE TMP_DIR "/lower"
>  #define LOWER_DATA LOWER_BASE "/data"
>  static const char lower_fl1[] = LOWER_DATA "/fl1";
> 
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> -- 
> 2.49.0
> 
> 

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

On 6/19/25 12:38, Mickaël Salaün wrote:
> On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:
>> This adds a test for the edge case discussed in [1], and in addition also
>> test rename operations when the operands are through disconnected paths,
>> as that go through a separate code path in Landlock.
>>
>> [1]: https://lore.kernel.org/linux-security-module/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org/
>>
>> This has resulted in a WARNING, due to collect_domain_accesses() not
>> expecting to reach a different root from path->mnt:
>>
>> 	#  RUN           layout1_bind.path_disconnected ...
>> 	#            OK  layout1_bind.path_disconnected
>> 	ok 96 layout1_bind.path_disconnected
>> 	#  RUN           layout1_bind.path_disconnected_rename ...
>> 	[..] ------------[ cut here ]------------
>> 	[..] WARNING: CPU: 3 PID: 385 at security/landlock/fs.c:1065 collect_domain_accesses
>> 	[..] ...
>> 	[..] RIP: 0010:collect_domain_accesses (security/landlock/fs.c:1065 (discriminator 2) security/landlock/fs.c:1031 (discriminator 2))
>> 	[..] current_check_refer_path (security/landlock/fs.c:1205)
>> 	[..] ...
>> 	[..] hook_path_rename (security/landlock/fs.c:1526)
>> 	[..] security_path_rename (security/security.c:2026 (discriminator 1))
>> 	[..] do_renameat2 (fs/namei.c:5264)
>> 	#            OK  layout1_bind.path_disconnected_rename
>> 	ok 97 layout1_bind.path_disconnected_rename
> 
> Good catch and thanks for the tests!  I sent a fix:
> https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/
> 
>>
>> My understanding is that terminating at the mountpoint is basically an
>> optimization, so that for rename operations we only walks the path from
>> the mountpoint to the real root once.  We probably want to keep this
>> optimization, as disconnected paths are probably a very rare edge case.
> 
> Rename operations can only happen within the same mount point, otherwise
> the kernel returns -EXDEV.  The collect_domain_accesses() is called for
> the source and the destination of a rename to walk to their common mount
> point, if any.  We could maybe improve this walk by doing them at the
> same time but because we don't know the depth of each path, I'm not sure
> the required extra complexity would be worth it.  The current approach
> is simple and opportunistically limits the walks.
> 
>>
>> This might need more thinking, but maybe if one of the operands is
>> disconnected, we can just let it walk until IS_ROOT(dentry), and also
>> collect access for the other path until IS_ROOT(dentry), then call
>> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
>> this case is_access_to_paths_allowed will not do any walking and just make
>> an access decision.)
> 
> If one side is in a disconnected directory and not the other side, the
> rename would be denied by the VFS,

Not always, right? For example in the path_disconnected_rename test we did:

5051.  ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
                             ^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^
                             Disconnected              Connected

(and it also has the other way)

So looks like as long as they are still reached from two fds with two
paths that have the same mnt, it will be allowed.  It's just that when we
do parent walk we end up missing the mount.  This also means that for this
refer check, if after doing the two separate walks (with the disconnected
side walking all the way to IS_ROOT), we then walk from mnt again, we
would allow the rename if there is a rule on mnt (or its parents) allowing
file creation and refers, even if the disconnected side technically now
lives outside the file hierarchy under mnt and does not have a parent with
a rule allowing file creation.

(I'm not saying this is necessary wrong or needs fixing, but I think it's
an interesting consequence of the current implementation.)

> but Landlock should still log (and then deny) the side that would be
> denied anyway.
> 
>>
>> Letting the walk continue until IS_ROOT(dentry) is what
>> is_access_to_paths_allowed() effectively does for non-renames.
>>
>> (Also note: moving the const char definitions a bit above so that we can
>> use the path for s4d1 in cleanup code.)
>>
>> Signed-off-by: Tingmao Wang <m@maowtm.org>
> 
> I squashed your patches and push them to my next branch with some minor
> changes.  Please let me know if there is something wrong.

Thanks for the edits!  I did notice two things:

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index fa0f18ec62c4..c0a54dde7225 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4561,6 +4561,17 @@ TEST_F_FORK(ioctl, handle_file_access_file)
 FIXTURE(layout1_bind) {};
 /* clang-format on */
 
+static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
+static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
+/* Moved targets for disconnected path tests. */
    ^^^^^^^^^^^^^
    I had "Move targets" here as a noun (i.e. the target/destinations of
    the renames)

+static const char dir_s4d1[] = TMP_DIR "/s4d1";
+static const char file1_s4d1[] = TMP_DIR "/s4d1/f1";
...

Also, I was just re-reading path_disconnected_rename and I managed to get
confused (i.e. "how is the rename in the forked child allowed at all (i.e.
how did we get EXDEV instead of EACCES) after applying layer 2?").  If you
end up amending that commit, can you add this short note:

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index c0a54dde7225..84615c4bb7c0 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4936,6 +4936,8 @@ TEST_F_FORK(layout1_bind, path_disconnected_rename)
 		},
 		{}
 	};
+
+	/* This layer only handles LANDLOCK_ACCESS_FS_READ_FILE only. */
 	const struct rule layer2_only_s1d2[] = {
 		{
 			.path = dir_s1d2,

Wish I had caught this earlier.  I mean neither of the two things are
hugely important, but I assume until you actually send the merge request
you can amend stuff relatively easily?  If not then it's also alright :)

>
> [...]

Ack to all suggestions, thanks!

Best,
Tingmao

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

On 6/22/25 16:42, Tingmao Wang wrote:
> +	/* This layer only handles LANDLOCK_ACCESS_FS_READ_FILE only. */
                                                               ^^^^^ sorry remove this

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Mickaël Salaün]

On Sun, Jun 22, 2025 at 04:42:49PM +0100, Tingmao Wang wrote:
> On 6/19/25 12:38, Mickaël Salaün wrote:
> > On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:
> >> This adds a test for the edge case discussed in [1], and in addition also
> >> test rename operations when the operands are through disconnected paths,
> >> as that go through a separate code path in Landlock.
> >>
> >> [1]: https://lore.kernel.org/linux-security-module/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org/
> >>
> >> This has resulted in a WARNING, due to collect_domain_accesses() not
> >> expecting to reach a different root from path->mnt:
> >>
> >> 	#  RUN           layout1_bind.path_disconnected ...
> >> 	#            OK  layout1_bind.path_disconnected
> >> 	ok 96 layout1_bind.path_disconnected
> >> 	#  RUN           layout1_bind.path_disconnected_rename ...
> >> 	[..] ------------[ cut here ]------------
> >> 	[..] WARNING: CPU: 3 PID: 385 at security/landlock/fs.c:1065 collect_domain_accesses
> >> 	[..] ...
> >> 	[..] RIP: 0010:collect_domain_accesses (security/landlock/fs.c:1065 (discriminator 2) security/landlock/fs.c:1031 (discriminator 2))
> >> 	[..] current_check_refer_path (security/landlock/fs.c:1205)
> >> 	[..] ...
> >> 	[..] hook_path_rename (security/landlock/fs.c:1526)
> >> 	[..] security_path_rename (security/security.c:2026 (discriminator 1))
> >> 	[..] do_renameat2 (fs/namei.c:5264)
> >> 	#            OK  layout1_bind.path_disconnected_rename
> >> 	ok 97 layout1_bind.path_disconnected_rename
> > 
> > Good catch and thanks for the tests!  I sent a fix:
> > https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/
> > 
> >>
> >> My understanding is that terminating at the mountpoint is basically an
> >> optimization, so that for rename operations we only walks the path from
> >> the mountpoint to the real root once.  We probably want to keep this
> >> optimization, as disconnected paths are probably a very rare edge case.
> > 
> > Rename operations can only happen within the same mount point, otherwise
> > the kernel returns -EXDEV.  The collect_domain_accesses() is called for
> > the source and the destination of a rename to walk to their common mount
> > point, if any.  We could maybe improve this walk by doing them at the
> > same time but because we don't know the depth of each path, I'm not sure
> > the required extra complexity would be worth it.  The current approach
> > is simple and opportunistically limits the walks.
> > 
> >>
> >> This might need more thinking, but maybe if one of the operands is
> >> disconnected, we can just let it walk until IS_ROOT(dentry), and also
> >> collect access for the other path until IS_ROOT(dentry), then call
> >> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
> >> this case is_access_to_paths_allowed will not do any walking and just make
> >> an access decision.)
> > 
> > If one side is in a disconnected directory and not the other side, the
> > rename would be denied by the VFS,
> 
> Not always, right? For example in the path_disconnected_rename test we did:

Correct, only the mount point matter.

> 
> 5051.  ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
>                              ^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^
>                              Disconnected              Connected
> 
> (and it also has the other way)
> 
> So looks like as long as they are still reached from two fds with two
> paths that have the same mnt, it will be allowed.  It's just that when we
> do parent walk we end up missing the mount.  This also means that for this
> refer check, if after doing the two separate walks (with the disconnected
> side walking all the way to IS_ROOT), we then walk from mnt again, we
> would allow the rename if there is a rule on mnt (or its parents) allowing
> file creation and refers, even if the disconnected side technically now
> lives outside the file hierarchy under mnt and does not have a parent with
> a rule allowing file creation.
> 
> (I'm not saying this is necessary wrong or needs fixing, but I think it's
> an interesting consequence of the current implementation.)

Hmm, that's indeed a very subtle side effect.  One issue with the
current implementation is that if a directory between the mount
point and the source has REFER, and another directory not part of the
source hierarchy but part of the disconnected directory's hierarchy has
REFER and no other directory has REFER, and either the source or the
destination hierarchy is disconnected between the mount point and the
directory with the REFER, then Landlock will still deny such
rename/link.  A directory with REFER initially between the mount point
and the disconnected directory would also be ignored.  There is also the
case where both the source and the destination are disconnected.

I didn't consider such cases with collect_domain_accesses().  I'm
wondering if this path walk gap should be fixed (instead of applying
https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/ )
or not.  We should not rely on optimization side effects, but I'm not
sure which behavior would make more sense...  Any though?

> 
> > but Landlock should still log (and then deny) the side that would be
> > denied anyway.
> > 
> >>
> >> Letting the walk continue until IS_ROOT(dentry) is what
> >> is_access_to_paths_allowed() effectively does for non-renames.
> >>
A> >> (Also note: moving the const char definitions a bit above so that we can
> >> use the path for s4d1 in cleanup code.)
> >>
> >> Signed-off-by: Tingmao Wang <m@maowtm.org>
> > 
> > I squashed your patches and push them to my next branch with some minor
> > changes.  Please let me know if there is something wrong.
> 
> Thanks for the edits!  I did notice two things:
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index fa0f18ec62c4..c0a54dde7225 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4561,6 +4561,17 @@ TEST_F_FORK(ioctl, handle_file_access_file)
>  FIXTURE(layout1_bind) {};
>  /* clang-format on */
>  
> +static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
> +static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
> +/* Moved targets for disconnected path tests. */
>     ^^^^^^^^^^^^^
>     I had "Move targets" here as a noun (i.e. the target/destinations of
>     the renames)

Makes sense now :)

> 
> +static const char dir_s4d1[] = TMP_DIR "/s4d1";
> +static const char file1_s4d1[] = TMP_DIR "/s4d1/f1";
> ...
> 
> Also, I was just re-reading path_disconnected_rename and I managed to get
> confused (i.e. "how is the rename in the forked child allowed at all (i.e.
> how did we get EXDEV instead of EACCES) after applying layer 2?").  If you
> end up amending that commit, can you add this short note:
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index c0a54dde7225..84615c4bb7c0 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4936,6 +4936,8 @@ TEST_F_FORK(layout1_bind, path_disconnected_rename)
>  		},
>  		{}
>  	};
> +
> +	/* This layer only handles LANDLOCK_ACCESS_FS_READ_FILE only. */

That can be useful.

>  	const struct rule layer2_only_s1d2[] = {
>  		{
>  			.path = dir_s1d2,
> 
> Wish I had caught this earlier.  I mean neither of the two things are
> hugely important, but I assume until you actually send the merge request
> you can amend stuff relatively easily?  If not then it's also alright :)

I apply your changes.  Commits should usually wait at least a week in
linux-next.

> 
> >
> > [...]
> 
> Ack to all suggestions, thanks!
> 
> Best,
> Tingmao
> 

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

On 6/23/25 20:40, Mickaël Salaün wrote:
> On Sun, Jun 22, 2025 at 04:42:49PM +0100, Tingmao Wang wrote:
>> On 6/19/25 12:38, Mickaël Salaün wrote:
>>> On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:
>>>> [...]
>>>>
>>>> This might need more thinking, but maybe if one of the operands is
>>>> disconnected, we can just let it walk until IS_ROOT(dentry), and also
>>>> collect access for the other path until IS_ROOT(dentry), then call
>>>> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
>>>> this case is_access_to_paths_allowed will not do any walking and just make
>>>> an access decision.)
>>>
>>> If one side is in a disconnected directory and not the other side, the
>>> rename would be denied by the VFS,
>>
>> Not always, right? For example in the path_disconnected_rename test we did:
> 
> Correct, only the mount point matter.
> 
>>
>> 5051.  ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
>>                              ^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^
>>                              Disconnected              Connected
>>
>> (and it also has the other way)
>>
>> So looks like as long as they are still reached from two fds with two
>> paths that have the same mnt, it will be allowed.  It's just that when we
>> do parent walk we end up missing the mount.  This also means that for this
>> refer check, if after doing the two separate walks (with the disconnected
>> side walking all the way to IS_ROOT), we then walk from mnt again, we
>> would allow the rename if there is a rule on mnt (or its parents) allowing
>> file creation and refers, even if the disconnected side technically now
>> lives outside the file hierarchy under mnt and does not have a parent with
>> a rule allowing file creation.
>>
>> (I'm not saying this is necessary wrong or needs fixing, but I think it's
>> an interesting consequence of the current implementation.)
> 
> Hmm, that's indeed a very subtle side effect.  One issue with the
> current implementation is that if a directory between the mount
> point and the source has REFER, and another directory not part of the
> source hierarchy but part of the disconnected directory's hierarchy has
> REFER and no other directory has REFER, and either the source or the
> destination hierarchy is disconnected between the mount point and the
> directory with the REFER, then Landlock will still deny such
> rename/link.  A directory with REFER initially between the mount point
> and the disconnected directory would also be ignored.  There is also the
> case where both the source and the destination are disconnected.

Sorry, I'm having trouble following this.  Can you maybe give a more
specific example, perhaps with commands?

By "mount point" do you mean the bind mount? If a path has became
disconnected because the directory moved away from under the mountpoint,
and is therefore not covered by any REFER (you said "either the source or
the destination hierarchy is disconnected between the mount point and the
directory with the REFER") wouldn't it make sense for the rename to be
denied?

> 
> I didn't consider such cases with collect_domain_accesses().  I'm
> wondering if this path walk gap should be fixed (instead of applying
> https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/ )
> or not.  We should not rely on optimization side effects, but I'm not
> sure which behavior would make more sense...  Any though?

I didn't quite understand your example above and how is it possible for us
to end up denying something that should be allowed.  My understanding of
the current implementation is, when either operands are disconnected, it
will walk all the way to the current filesystem's root and stop there.
However, it will then still do the walk from the original bind mount up to
the real root (/), and if there is any REFER rules on that path, we will
still allow the rename.  This means that if the rename still ends up being
denied, then it wouldn't have been allowed in the first place, even if the
path has not become disconnected.

An interesting concrete example I came up with:

/# uname -a
Linux 5610c72ba8a0 6.16.0-rc2-dev #43 SMP ...
/# mkdir /a /b
/# mkdir /a/a1 /b/b1
/# mount -t tmpfs none /a/a1
/# mkdir /a/a1/a11
/# mount --bind /a/a1/a11 /b/b1
/# mkdir /a/a1/a11/a111
/# tree /a /b
/a
`-- a1
    `-- a11
        `-- a111
/b
`-- b1
    `-- a111

7 directories, 0 files
/# cd /b/b1/a111/
/b/b1/a111# mv /a/a1/a11/a111 /a/a1/a12
/b/b1/a111# ls ..  # we're disconnected now
ls: cannot access '..': No such file or directory
/b/b1/a111 [2]# touch /a/a1/a12/file

/b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer ls
Executing the sandboxed command...
file

/b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer mv -v file file2
Executing the sandboxed command...
mv: cannot move 'file' to 'file2': Permission denied
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# This fails because for same dir rename we just use is_access_to_path_allowed,
# which will stop at /a/a1 (and thus never reach either /b/b1 or /).

/b/b1/a111 [1]# mkdir subdir
/b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/b/b1  /sandboxer mv -v file subdir/file2
Executing the sandboxed command...
[..] WARNING: CPU: 1 PID: 656 at security/landlock/fs.c:1065 ...
renamed 'file' -> 'subdir/file2'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# This works because now we restart walk from /b/b1 (the bind mnt)

/b/b1/a111# mv subdir/file2 file
/b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/a  /sandboxer mv -v file subdir/file2
Executing the sandboxed command...
mv: cannot move 'file' to 'subdir/file2': Permission denied
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# This is also not allowed, but that's OK since even though technically we're
# actually moving /a/a1/a12/file to /a/a1/a12/subdir/file2, we're not doing it
# through /a (we're walking into a12 via /b/b1, so rules on /a shouldn't
# apply anyway)

/b/b1/a111 [1]# LL_FS_RO=/:/a/a1 LL_FS_RW=/b  /sandboxer mv -v file subdir/file2
Executing the sandboxed command...
renamed 'file' -> 'subdir/file2'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# And this works because we walk from /b/b1 after doing collect_domain_accesses

I think overall this is just a very strange edge case and people should
not rely on the exact behavior whether it's intentional or optimization
side-effect (as long as it deny access / renames when there is no rules at
any of the reasonable upper directories).  Also, since as far as I can
tell this "optimization" only accidentally allows more access (i.e.  rules
anywhere between the bind mountpoint to real root would apply, even if
technically the now disconnected directory belongs outside of the
mountpoint), I think it might be fine to leave it as-is, rather than
potentially complicating this code to deal with this quite unusual edge
case?  (I mean, it's not exactly obvious to me whether it is more correct
to respect rules placed between the original bind mountpoint and root, or
more correct to ignore these rules (i.e. the behaviour of non-refer access
checks))

It is a bit weird that `mv -v file file2` and `mv -v file subdir/file2`
behaves differently tho.

If you would like to fix it, what do you think about my initial idea?:
> This might need more thinking, but maybe if one of the operands is
> disconnected, we can just let it walk until IS_ROOT(dentry), and also
> collect access for the other path until IS_ROOT(dentry), then call
> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
> this case is_access_to_paths_allowed will not do any walking and just make
> an access decision.)

This will basically make the refer checks behave the same as non-refer
checks on disconnected paths - walk until IS_ROOT, and stop there.

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Mickaël Salaün]

On Tue, Jun 24, 2025 at 12:16:55AM +0100, Tingmao Wang wrote:
> On 6/23/25 20:40, Mickaël Salaün wrote:
> > On Sun, Jun 22, 2025 at 04:42:49PM +0100, Tingmao Wang wrote:
> >> On 6/19/25 12:38, Mickaël Salaün wrote:
> >>> On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:
> >>>> [...]
> >>>>
> >>>> This might need more thinking, but maybe if one of the operands is
> >>>> disconnected, we can just let it walk until IS_ROOT(dentry), and also
> >>>> collect access for the other path until IS_ROOT(dentry), then call
> >>>> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
> >>>> this case is_access_to_paths_allowed will not do any walking and just make
> >>>> an access decision.)
> >>>
> >>> If one side is in a disconnected directory and not the other side, the
> >>> rename would be denied by the VFS,
> >>
> >> Not always, right? For example in the path_disconnected_rename test we did:
> > 
> > Correct, only the mount point matter.
> > 
> >>
> >> 5051.  ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
> >>                              ^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^
> >>                              Disconnected              Connected
> >>
> >> (and it also has the other way)
> >>
> >> So looks like as long as they are still reached from two fds with two
> >> paths that have the same mnt, it will be allowed.  It's just that when we
> >> do parent walk we end up missing the mount.  This also means that for this
> >> refer check, if after doing the two separate walks (with the disconnected
> >> side walking all the way to IS_ROOT), we then walk from mnt again, we
> >> would allow the rename if there is a rule on mnt (or its parents) allowing
> >> file creation and refers, even if the disconnected side technically now
> >> lives outside the file hierarchy under mnt and does not have a parent with
> >> a rule allowing file creation.
> >>
> >> (I'm not saying this is necessary wrong or needs fixing, but I think it's
> >> an interesting consequence of the current implementation.)
> > 
> > Hmm, that's indeed a very subtle side effect.  One issue with the
> > current implementation is that if a directory between the mount
> > point and the source has REFER, and another directory not part of the
> > source hierarchy but part of the disconnected directory's hierarchy has
> > REFER and no other directory has REFER, and either the source or the
> > destination hierarchy is disconnected between the mount point and the
> > directory with the REFER, then Landlock will still deny such
> > rename/link.  A directory with REFER initially between the mount point
> > and the disconnected directory would also be ignored.  There is also the
> > case where both the source and the destination are disconnected.
> 
> Sorry, I'm having trouble following this.  Can you maybe give a more
> specific example, perhaps with commands?

Let's say we initially have this hierarchy:

root
├── s1d1
│   └── s1d2 [REFER]
│       └── s1d3
│           └── s1d4
│               └── f1
├── s2d1 [bind mount of s1d1]
│   └── s1d2 [REFER]
│       └── s1d3
│           └── s1d4
│               └── f1
└── s3d1 [REFER]

s1d3 has s1d2 as parent with the REFER right.

We open [fd:s1d4].

Now, s1d1/s1d2/s1d3 is moved to s3d1/s1d3, which makes [fd:s1d4]/..
disconnected:

root
├── s1d1
│   └── s1d2 [REFER]
├── s2d1 [bind mount of s1d1]
│   └── s1d2 [REFER]
└── s3d1 [REFER]
    └── s1d3 [moved from s1d2]
        └── s1d4
            └── f1

Now, s1d3 has s3d1 as parent with the REFER right.

Moving [fd:s1d4]/f1 to s2d1/s1d2/f1 would be deny by Landlock whereas
the source and destination had and still have REFER in their
hierarchies.  This is because s3d1 and s1d2 are evaluated for
[fd:s1d4]/f1.  We could have a similar scenario for the destination and
for both.


> 
> By "mount point" do you mean the bind mount? If a path has became
> disconnected because the directory moved away from under the mountpoint,
> and is therefore not covered by any REFER (you said "either the source or
> the destination hierarchy is disconnected between the mount point and the
> directory with the REFER") wouldn't it make sense for the rename to be
> denied?

Yes if the new hierarchy (e.g. s3d1 in my example) doesn't have REFER.

> 
> > 
> > I didn't consider such cases with collect_domain_accesses().  I'm
> > wondering if this path walk gap should be fixed (instead of applying
> > https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/ )
> > or not.  We should not rely on optimization side effects, but I'm not
> > sure which behavior would make more sense...  Any though?
> 
> I didn't quite understand your example above and how is it possible for us
> to end up denying something that should be allowed.  My understanding of
> the current implementation is, when either operands are disconnected, it
> will walk all the way to the current filesystem's root and stop there.
> However, it will then still do the walk from the original bind mount up to
> the real root (/), and if there is any REFER rules on that path, we will
> still allow the rename.  This means that if the rename still ends up being
> denied, then it wouldn't have been allowed in the first place, even if the
> path has not become disconnected.
> 
> An interesting concrete example I came up with:
> 
> /# uname -a
> Linux 5610c72ba8a0 6.16.0-rc2-dev #43 SMP ...
> /# mkdir /a /b
> /# mkdir /a/a1 /b/b1
> /# mount -t tmpfs none /a/a1
> /# mkdir /a/a1/a11
> /# mount --bind /a/a1/a11 /b/b1
> /# mkdir /a/a1/a11/a111
> /# tree /a /b
> /a
> `-- a1
>     `-- a11
>         `-- a111
> /b
> `-- b1
>     `-- a111
> 
> 7 directories, 0 files
> /# cd /b/b1/a111/
> /b/b1/a111# mv /a/a1/a11/a111 /a/a1/a12
> /b/b1/a111# ls ..  # we're disconnected now
> ls: cannot access '..': No such file or directory
> /b/b1/a111 [2]# touch /a/a1/a12/file
> 
> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer ls
> Executing the sandboxed command...
> file
> 
> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer mv -v file file2
> Executing the sandboxed command...
> mv: cannot move 'file' to 'file2': Permission denied
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # This fails because for same dir rename we just use is_access_to_path_allowed,
> # which will stop at /a/a1 (and thus never reach either /b/b1 or /).

Good example, and this rename should probably be allowed because the
root directory allows REFER.

> 
> /b/b1/a111 [1]# mkdir subdir
> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/b/b1  /sandboxer mv -v file subdir/file2
> Executing the sandboxed command...
> [..] WARNING: CPU: 1 PID: 656 at security/landlock/fs.c:1065 ...
> renamed 'file' -> 'subdir/file2'
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # This works because now we restart walk from /b/b1 (the bind mnt)
> 
> /b/b1/a111# mv subdir/file2 file
> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/a  /sandboxer mv -v file subdir/file2
> Executing the sandboxed command...
> mv: cannot move 'file' to 'subdir/file2': Permission denied
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # This is also not allowed, but that's OK since even though technically we're
> # actually moving /a/a1/a12/file to /a/a1/a12/subdir/file2, we're not doing it
> # through /a (we're walking into a12 via /b/b1, so rules on /a shouldn't
> # apply anyway)

Yes

> 
> /b/b1/a111 [1]# LL_FS_RO=/:/a/a1 LL_FS_RW=/b  /sandboxer mv -v file subdir/file2
> Executing the sandboxed command...
> renamed 'file' -> 'subdir/file2'
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # And this works because we walk from /b/b1 after doing collect_domain_accesses
> 
> I think overall this is just a very strange edge case and people should
> not rely on the exact behavior whether it's intentional or optimization
> side-effect (as long as it deny access / renames when there is no rules at
> any of the reasonable upper directories).  Also, since as far as I can
> tell this "optimization" only accidentally allows more access (i.e.  rules
> anywhere between the bind mountpoint to real root would apply, even if
> technically the now disconnected directory belongs outside of the
> mountpoint), I think it might be fine to leave it as-is, rather than
> potentially complicating this code to deal with this quite unusual edge
> case?  (I mean, it's not exactly obvious to me whether it is more correct
> to respect rules placed between the original bind mountpoint and root, or
> more correct to ignore these rules (i.e. the behaviour of non-refer access
> checks))

I kind of agree, overall it's not really a security issue (if we
consider the origin of files), but it may still be inconsistent for
users in rare cases.  Anyway, even if we don't want it, users could rely
on this edge case (cf. Hyrum's law).

> 
> It is a bit weird that `mv -v file file2` and `mv -v file subdir/file2`
> behaves differently tho.

Yes, `mv file file2` doesn't depends on REFER because it cannot impact a
Landlock security policy.  This behavior is a bit weird without kernel
and Landlock knowledge though.

> 
> If you would like to fix it, what do you think about my initial idea?:
> > This might need more thinking, but maybe if one of the operands is
> > disconnected, we can just let it walk until IS_ROOT(dentry), and also
> > collect access for the other path until IS_ROOT(dentry), then call

Until then, it would be unchanged, right?

> > is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
> > this case is_access_to_paths_allowed will not do any walking and just make
> > an access decision.)

Are you suggesting to not evaluate mnt_dir for disconnected paths?  What
about the case where both the source and the destination are
disconnected?

> 
> This will basically make the refer checks behave the same as non-refer
> checks on disconnected paths - walk until IS_ROOT, and stop there.

I think it would make more sense indeed.

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Tingmao Wang]

On 6/25/25 15:52, Mickaël Salaün wrote:
> On Tue, Jun 24, 2025 at 12:16:55AM +0100, Tingmao Wang wrote:
>> [..]
> 
> Let's say we initially have this hierarchy:
> 
> root
> ├── s1d1
> │   └── s1d2 [REFER]
> │       └── s1d3
> │           └── s1d4
> │               └── f1
> ├── s2d1 [bind mount of s1d1]
> │   └── s1d2 [REFER]
> │       └── s1d3
> │           └── s1d4
> │               └── f1
> └── s3d1 [REFER]
> 
> s1d3 has s1d2 as parent with the REFER right.
> 
> We open [fd:s1d4].
> 
> Now, s1d1/s1d2/s1d3 is moved to s3d1/s1d3, which makes [fd:s1d4]/..
> disconnected:
> 
> root
> ├── s1d1
> │   └── s1d2 [REFER]
> ├── s2d1 [bind mount of s1d1]
> │   └── s1d2 [REFER]
> └── s3d1 [REFER]
>     └── s1d3 [moved from s1d2]
>         └── s1d4
>             └── f1
> 
> Now, s1d3 has s3d1 as parent with the REFER right.
> 
> Moving [fd:s1d4]/f1 to s2d1/s1d2/f1 would be deny by Landlock

Maybe I'm misunderstanding your description, but this seems to work for
me?

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d8f9259fffe4..5e550e6da49c 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -5201,6 +5201,72 @@ TEST_F_FORK(layout1_bind, path_disconnected_link)
 	}
 }
 
+FIXTURE(layout_disconnected_special){};
+FIXTURE_SETUP(layout_disconnected_special)
+{
+	prepare_layout(_metadata);
+
+	create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d4/f1");
+	create_directory(_metadata, TMP_DIR "/s2d1");
+	create_directory(_metadata, TMP_DIR "/s3d1");
+
+	set_cap(_metadata, CAP_SYS_ADMIN);
+	ASSERT_EQ(0,
+		  mount(TMP_DIR "/s1d1", TMP_DIR "/s2d1", NULL, MS_BIND, NULL));
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN_PARENT(layout_disconnected_special)
+{
+	/* umount(TMP_DIR "/s2d1") is handled by namespace lifetime. */
+
+	remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d4/f1");
+
+	cleanup_layout(_metadata);
+}
+
+TEST_F_FORK(layout_disconnected_special, disconnected_special)
+{
+	const __u64 access =
+		LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG |
+		LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_READ_FILE;
+	const struct rule rules[] = {
+		{
+			.path = TMP_DIR "/s1d1/s1d2",
+			.access = access,
+		},
+		{
+			.path = TMP_DIR "/s3d1",
+			.access = access,
+		},
+		{},
+	};
+	int s1d4_bind_fd, ruleset_fd;
+
+	s1d4_bind_fd = open(TMP_DIR "/s2d1/s1d2/s1d3/s1d4", O_PATH | O_CLOEXEC);
+	EXPECT_LE(0, s1d4_bind_fd);
+
+	ruleset_fd = create_ruleset(_metadata, access, rules);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Make it disconnected. */
+	EXPECT_EQ(0, renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d3", AT_FDCWD,
+			      TMP_DIR "/s3d1/s1d3"));
+
+	/* Check it's disconnected. */
+	ASSERT_EQ(ENOENT, test_open_rel(s1d4_bind_fd, "..", O_DIRECTORY));
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(0, renameat(s1d4_bind_fd, "f1", AT_FDCWD,
+			      TMP_DIR "/s2d1/s1d2/f1"));
+	EXPECT_EQ(0, test_open(TMP_DIR "/s2d1/s1d2/f1", O_RDONLY));
+	EXPECT_EQ(0, renameat(AT_FDCWD, TMP_DIR "/s2d1/s1d2/f1", s1d4_bind_fd,
+			      "f1"));
+	EXPECT_EQ(0, test_open(TMP_DIR "/s3d1/s1d3/s1d4/f1", O_RDONLY));
+}
+
 #define LOWER_BASE TMP_DIR "/lower"
 #define LOWER_DATA LOWER_BASE "/data"
 static const char lower_fl1[] = LOWER_DATA "/fl1";

Output:

	root@5610c72ba8a0 /t/landlock# cp /linux/tools/testing/selftests/landlock/ /tmp -r
	root@5610c72ba8a0 /t/landlock# ./fs_test -t disconnected_special
	TAP version 13
	1..1
	# Starting 1 tests from 1 test cases.
	#  RUN           layout_disconnected_special.disconnected_special ...
	#            OK  layout_disconnected_special.disconnected_special
	ok 1 layout_disconnected_special.disconnected_special
	# PASSED: 1 / 1 tests passed.
	# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0

(I think this is similar to an existing test case, but if you think it's
worth having, feel free to add it to the commit (maybe it needs a better
name than disconnected_special))

I tested this manually initially which would have been on virtiofs instead
of tmpfs, and the behaviour is the same - rename was allowed.

> whereas
> the source and destination had and still have REFER in their
> hierarchies.  This is because s3d1 and s1d2 are evaluated for
> [fd:s1d4]/f1.  We could have a similar scenario for the destination and
> for both.
> 
> [...]
>> An interesting concrete example I came up with:
>>
>> /# uname -a
>> Linux 5610c72ba8a0 6.16.0-rc2-dev #43 SMP ...
>> /# mkdir /a /b
>> /# mkdir /a/a1 /b/b1
>> /# mount -t tmpfs none /a/a1
>> /# mkdir /a/a1/a11
>> /# mount --bind /a/a1/a11 /b/b1
>> /# mkdir /a/a1/a11/a111
>> /# tree /a /b
>> /a
>> `-- a1
>>     `-- a11
>>         `-- a111
>> /b
>> `-- b1
>>     `-- a111
>>
>> 7 directories, 0 files
>> /# cd /b/b1/a111/
>> /b/b1/a111# mv /a/a1/a11/a111 /a/a1/a12
>> /b/b1/a111# ls ..  # we're disconnected now
>> ls: cannot access '..': No such file or directory
>> /b/b1/a111 [2]# touch /a/a1/a12/file
>>
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer ls
>> Executing the sandboxed command...
>> file
>>
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer mv -v file file2
>> Executing the sandboxed command...
>> mv: cannot move 'file' to 'file2': Permission denied
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # This fails because for same dir rename we just use is_access_to_path_allowed,
>> # which will stop at /a/a1 (and thus never reach either /b/b1 or /).
> 
> Good example, and this rename should probably be allowed because the
> root directory allows REFER.

Well, it is disallowed for the same reason why a read to [disconnected
cwd]/file would be disallowed, even if root has an allow everything rule -
landlock never gets to the root because this is on a separate filesystem,
and so if the path is disconnected it can't get out of it.

> 
>>
>> /b/b1/a111 [1]# mkdir subdir
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/b/b1  /sandboxer mv -v file subdir/file2
>> Executing the sandboxed command...
>> [..] WARNING: CPU: 1 PID: 656 at security/landlock/fs.c:1065 ...
>> renamed 'file' -> 'subdir/file2'
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # This works because now we restart walk from /b/b1 (the bind mnt)
>>
>> /b/b1/a111# mv subdir/file2 file
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/a  /sandboxer mv -v file subdir/file2
>> Executing the sandboxed command...
>> mv: cannot move 'file' to 'subdir/file2': Permission denied
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # This is also not allowed, but that's OK since even though technically we're
>> # actually moving /a/a1/a12/file to /a/a1/a12/subdir/file2, we're not doing it
>> # through /a (we're walking into a12 via /b/b1, so rules on /a shouldn't
>> # apply anyway)
> 
> Yes
> 
>>
>> /b/b1/a111 [1]# LL_FS_RO=/:/a/a1 LL_FS_RW=/b  /sandboxer mv -v file subdir/file2
>> Executing the sandboxed command...
>> renamed 'file' -> 'subdir/file2'
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # And this works because we walk from /b/b1 after doing collect_domain_accesses
>>
>> I think overall this is just a very strange edge case and people should
>> not rely on the exact behavior whether it's intentional or optimization
>> side-effect (as long as it deny access / renames when there is no rules at
>> any of the reasonable upper directories).  Also, since as far as I can
>> tell this "optimization" only accidentally allows more access (i.e.  rules
>> anywhere between the bind mountpoint to real root would apply, even if
>> technically the now disconnected directory belongs outside of the
>> mountpoint), I think it might be fine to leave it as-is, rather than
>> potentially complicating this code to deal with this quite unusual edge
>> case?  (I mean, it's not exactly obvious to me whether it is more correct
>> to respect rules placed between the original bind mountpoint and root, or
>> more correct to ignore these rules (i.e. the behaviour of non-refer access
>> checks))
> 
> I kind of agree, overall it's not really a security issue (if we
> consider the origin of files), but it may still be inconsistent for
> users in rare cases.  Anyway, even if we don't want it, users could rely
> on this edge case (cf. Hyrum's law).
> 
>>
>> It is a bit weird that `mv -v file file2` and `mv -v file subdir/file2`
>> behaves differently tho.
> 
> Yes, `mv file file2` doesn't depends on REFER because it cannot impact a
> Landlock security policy.  This behavior is a bit weird without kernel
> and Landlock knowledge though.
> 
>>
>> If you would like to fix it, what do you think about my initial idea?:
>>> This might need more thinking, but maybe if one of the operands is
>>> disconnected, we can just let it walk until IS_ROOT(dentry), and also
>>> collect access for the other path until IS_ROOT(dentry), then call
> 
> Until then, it would be unchanged, right?

Sorry, I'm not fully clear on what you meant by "until then", and what
would be unchanged, but on second thought I think this proposal is
problematic as it would mean that we won't follow_up on mountpoints even
for the other connected path (as collect_domain_accesses does not do
that).

> 
>>> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
>>> this case is_access_to_paths_allowed will not do any walking and just make
>>> an access decision.)
> 
> Are you suggesting to not evaluate mnt_dir for disconnected paths?  What
> about the case where both the source and the destination are
> disconnected?

Yes basically, but I think my proposal was problematic.

> 
>>
>> This will basically make the refer checks behave the same as non-refer
>> checks on disconnected paths - walk until IS_ROOT, and stop there.
> 
> I think it would make more sense indeed.

It would make sense if both sides are disconnected, but not if just one
side is.  In that case we still want to walk the other connected path
normally.

Re: [PATCH] selftests/landlock: Add tests for access through disconnected paths

[Mickaël Salaün]

On Wed, Jun 25, 2025 at 09:46:08PM +0100, Tingmao Wang wrote:
> On 6/25/25 15:52, Mickaël Salaün wrote:
> > On Tue, Jun 24, 2025 at 12:16:55AM +0100, Tingmao Wang wrote:
> >> [..]
> > 
> > Let's say we initially have this hierarchy:
> > 
> > root
> > ├── s1d1
> > │   └── s1d2 [REFER]
> > │       └── s1d3
> > │           └── s1d4
> > │               └── f1
> > ├── s2d1 [bind mount of s1d1]
> > │   └── s1d2 [REFER]
> > │       └── s1d3
> > │           └── s1d4
> > │               └── f1
> > └── s3d1 [REFER]
> > 
> > s1d3 has s1d2 as parent with the REFER right.
> > 
> > We open [fd:s1d4].
> > 
> > Now, s1d1/s1d2/s1d3 is moved to s3d1/s1d3, which makes [fd:s1d4]/..
> > disconnected:
> > 
> > root
> > ├── s1d1
> > │   └── s1d2 [REFER]
> > ├── s2d1 [bind mount of s1d1]
> > │   └── s1d2 [REFER]
> > └── s3d1 [REFER]
> >     └── s1d3 [moved from s1d2]
> >         └── s1d4
> >             └── f1
> > 
> > Now, s1d3 has s3d1 as parent with the REFER right.
> > 
> > Moving [fd:s1d4]/f1 to s2d1/s1d2/f1 would be deny by Landlock
> 
> Maybe I'm misunderstanding your description, but this seems to work for
> me?

You're right, this is a wrong example.

> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index d8f9259fffe4..5e550e6da49c 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -5201,6 +5201,72 @@ TEST_F_FORK(layout1_bind, path_disconnected_link)
>  	}
>  }
>  
> +FIXTURE(layout_disconnected_special){};
> +FIXTURE_SETUP(layout_disconnected_special)
> +{
> +	prepare_layout(_metadata);
> +
> +	create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d4/f1");
> +	create_directory(_metadata, TMP_DIR "/s2d1");
> +	create_directory(_metadata, TMP_DIR "/s3d1");
> +
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	ASSERT_EQ(0,
> +		  mount(TMP_DIR "/s1d1", TMP_DIR "/s2d1", NULL, MS_BIND, NULL));
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +}
> +
> +FIXTURE_TEARDOWN_PARENT(layout_disconnected_special)
> +{
> +	/* umount(TMP_DIR "/s2d1") is handled by namespace lifetime. */
> +
> +	remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d4/f1");
> +
> +	cleanup_layout(_metadata);
> +}
> +
> +TEST_F_FORK(layout_disconnected_special, disconnected_special)
> +{
> +	const __u64 access =
> +		LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG |
> +		LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_READ_FILE;
> +	const struct rule rules[] = {
> +		{
> +			.path = TMP_DIR "/s1d1/s1d2",
> +			.access = access,
> +		},
> +		{
> +			.path = TMP_DIR "/s3d1",
> +			.access = access,
> +		},
> +		{},
> +	};
> +	int s1d4_bind_fd, ruleset_fd;
> +
> +	s1d4_bind_fd = open(TMP_DIR "/s2d1/s1d2/s1d3/s1d4", O_PATH | O_CLOEXEC);
> +	EXPECT_LE(0, s1d4_bind_fd);
> +
> +	ruleset_fd = create_ruleset(_metadata, access, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	/* Make it disconnected. */
> +	EXPECT_EQ(0, renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d3", AT_FDCWD,
> +			      TMP_DIR "/s3d1/s1d3"));
> +
> +	/* Check it's disconnected. */
> +	ASSERT_EQ(ENOENT, test_open_rel(s1d4_bind_fd, "..", O_DIRECTORY));
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +
> +	EXPECT_EQ(0, renameat(s1d4_bind_fd, "f1", AT_FDCWD,
> +			      TMP_DIR "/s2d1/s1d2/f1"));
> +	EXPECT_EQ(0, test_open(TMP_DIR "/s2d1/s1d2/f1", O_RDONLY));
> +	EXPECT_EQ(0, renameat(AT_FDCWD, TMP_DIR "/s2d1/s1d2/f1", s1d4_bind_fd,
> +			      "f1"));
> +	EXPECT_EQ(0, test_open(TMP_DIR "/s3d1/s1d3/s1d4/f1", O_RDONLY));
> +}
> +
>  #define LOWER_BASE TMP_DIR "/lower"
>  #define LOWER_DATA LOWER_BASE "/data"
>  static const char lower_fl1[] = LOWER_DATA "/fl1";
> 
> Output:
> 
> 	root@5610c72ba8a0 /t/landlock# cp /linux/tools/testing/selftests/landlock/ /tmp -r
> 	root@5610c72ba8a0 /t/landlock# ./fs_test -t disconnected_special
> 	TAP version 13
> 	1..1
> 	# Starting 1 tests from 1 test cases.
> 	#  RUN           layout_disconnected_special.disconnected_special ...
> 	#            OK  layout_disconnected_special.disconnected_special
> 	ok 1 layout_disconnected_special.disconnected_special
> 	# PASSED: 1 / 1 tests passed.
> 	# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> (I think this is similar to an existing test case, but if you think it's
> worth having, feel free to add it to the commit (maybe it needs a better
> name than disconnected_special))
> 
> I tested this manually initially which would have been on virtiofs instead
> of tmpfs, and the behaviour is the same - rename was allowed.
> 
> > whereas
> > the source and destination had and still have REFER in their
> > hierarchies.  This is because s3d1 and s1d2 are evaluated for
> > [fd:s1d4]/f1.  We could have a similar scenario for the destination and
> > for both.
> > 
> > [...]
> >> An interesting concrete example I came up with:
> >>
> >> /# uname -a
> >> Linux 5610c72ba8a0 6.16.0-rc2-dev #43 SMP ...
> >> /# mkdir /a /b
> >> /# mkdir /a/a1 /b/b1
> >> /# mount -t tmpfs none /a/a1
> >> /# mkdir /a/a1/a11
> >> /# mount --bind /a/a1/a11 /b/b1
> >> /# mkdir /a/a1/a11/a111
> >> /# tree /a /b
> >> /a
> >> `-- a1
> >>     `-- a11
> >>         `-- a111
> >> /b
> >> `-- b1
> >>     `-- a111
> >>
> >> 7 directories, 0 files
> >> /# cd /b/b1/a111/
> >> /b/b1/a111# mv /a/a1/a11/a111 /a/a1/a12
> >> /b/b1/a111# ls ..  # we're disconnected now
> >> ls: cannot access '..': No such file or directory
> >> /b/b1/a111 [2]# touch /a/a1/a12/file
> >>
> >> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer ls
> >> Executing the sandboxed command...
> >> file
> >>
> >> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1  /sandboxer mv -v file file2
> >> Executing the sandboxed command...
> >> mv: cannot move 'file' to 'file2': Permission denied
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> # This fails because for same dir rename we just use is_access_to_path_allowed,
> >> # which will stop at /a/a1 (and thus never reach either /b/b1 or /).
> > 
> > Good example, and this rename should probably be allowed because the
> > root directory allows REFER.
> 
> Well, it is disallowed for the same reason why a read to [disconnected
> cwd]/file would be disallowed, even if root has an allow everything rule -
> landlock never gets to the root because this is on a separate filesystem,
> and so if the path is disconnected it can't get out of it.
> 
> > 
> >>
> >> /b/b1/a111 [1]# mkdir subdir
> >> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/b/b1  /sandboxer mv -v file subdir/file2
> >> Executing the sandboxed command...
> >> [..] WARNING: CPU: 1 PID: 656 at security/landlock/fs.c:1065 ...
> >> renamed 'file' -> 'subdir/file2'
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> # This works because now we restart walk from /b/b1 (the bind mnt)
> >>
> >> /b/b1/a111# mv subdir/file2 file
> >> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/a  /sandboxer mv -v file subdir/file2
> >> Executing the sandboxed command...
> >> mv: cannot move 'file' to 'subdir/file2': Permission denied
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> # This is also not allowed, but that's OK since even though technically we're
> >> # actually moving /a/a1/a12/file to /a/a1/a12/subdir/file2, we're not doing it
> >> # through /a (we're walking into a12 via /b/b1, so rules on /a shouldn't
> >> # apply anyway)
> > 
> > Yes
> > 
> >>
> >> /b/b1/a111 [1]# LL_FS_RO=/:/a/a1 LL_FS_RW=/b  /sandboxer mv -v file subdir/file2
> >> Executing the sandboxed command...
> >> renamed 'file' -> 'subdir/file2'
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> # And this works because we walk from /b/b1 after doing collect_domain_accesses
> >>
> >> I think overall this is just a very strange edge case and people should
> >> not rely on the exact behavior whether it's intentional or optimization
> >> side-effect (as long as it deny access / renames when there is no rules at
> >> any of the reasonable upper directories).  Also, since as far as I can
> >> tell this "optimization" only accidentally allows more access (i.e.  rules
> >> anywhere between the bind mountpoint to real root would apply, even if
> >> technically the now disconnected directory belongs outside of the
> >> mountpoint), I think it might be fine to leave it as-is, rather than
> >> potentially complicating this code to deal with this quite unusual edge
> >> case?  (I mean, it's not exactly obvious to me whether it is more correct
> >> to respect rules placed between the original bind mountpoint and root, or
> >> more correct to ignore these rules (i.e. the behaviour of non-refer access
> >> checks))
> > 
> > I kind of agree, overall it's not really a security issue (if we
> > consider the origin of files), but it may still be inconsistent for
> > users in rare cases.  Anyway, even if we don't want it, users could rely
> > on this edge case (cf. Hyrum's law).
> > 
> >>
> >> It is a bit weird that `mv -v file file2` and `mv -v file subdir/file2`
> >> behaves differently tho.
> > 
> > Yes, `mv file file2` doesn't depends on REFER because it cannot impact a
> > Landlock security policy.  This behavior is a bit weird without kernel
> > and Landlock knowledge though.
> > 
> >>
> >> If you would like to fix it, what do you think about my initial idea?:
> >>> This might need more thinking, but maybe if one of the operands is
> >>> disconnected, we can just let it walk until IS_ROOT(dentry), and also
> >>> collect access for the other path until IS_ROOT(dentry), then call
> > 
> > Until then, it would be unchanged, right?
> 
> Sorry, I'm not fully clear on what you meant by "until then", and what
> would be unchanged, but on second thought I think this proposal is
> problematic as it would mean that we won't follow_up on mountpoints even
> for the other connected path (as collect_domain_accesses does not do
> that).
> 
> > 
> >>> is_access_to_paths_allowed() passing in the root dentry we walked to?  (In
> >>> this case is_access_to_paths_allowed will not do any walking and just make
> >>> an access decision.)
> > 
> > Are you suggesting to not evaluate mnt_dir for disconnected paths?  What
> > about the case where both the source and the destination are
> > disconnected?
> 
> Yes basically, but I think my proposal was problematic.
> 
> > 
> >>
> >> This will basically make the refer checks behave the same as non-refer
> >> checks on disconnected paths - walk until IS_ROOT, and stop there.
> > 
> > I think it would make more sense indeed.
> 
> It would make sense if both sides are disconnected, but not if just one
> side is.  In that case we still want to walk the other connected path
> normally.

This discussion made me think about different edge cases and I sent a
proposal for a fix with tests that should cover all the cases we talked
about here:
https://lore.kernel.org/all/20250701183812.3201231-1-mic@digikod.net/