On Sun, Jun 22, 2025 at 04:42:49PM +0100, Tingmao Wang wrote:

On 6/19/25 12:38, Mickaël Salaün wrote:

quoted

On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:

quoted

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/ (local)

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/ (local)

quoted

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.

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
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/ (local) )
or not.  We should not rely on optimization side effects, but I'm not
sure which behavior would make more sense...  Any though?

quoted

but Landlock should still log (and then deny) the side that would be
denied anyway.

quoted

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

Makes sense now :)

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.

quoted

[...]

Ack to all suggestions, thanks!

Best,
Tingmao