[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 at 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