On 6/25/25 15:52, Mickaël Salaün wrote:

On Tue, Jun 24, 2025 at 12:16:55AM +0100, Tingmao Wang wrote:

quoted

[..]

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.

[...]

quoted

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.

quoted

/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

quoted

/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).

quoted

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.

quoted

If you would like to fix it, what do you think about my initial idea?:

quoted

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

quoted

quoted

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.

quoted

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.