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

No functional changes intended.

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

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 6fee7c20f64d..3adac544dc9e 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -837,8 +837,8 @@ static bool is_access_to_paths_allowed(
 	 * restriction.
 	 */
 	while (true) {
-		struct dentry *parent_dentry;
 		const struct landlock_rule *rule;
+		struct path root = {};
 
 		/*
 		 * If at least all accesses allowed on the destination are
@@ -895,34 +895,23 @@ static bool is_access_to_paths_allowed(
 		/* Stops when a rule from each layer grants access. */
 		if (allowed_parent1 && allowed_parent2)
 			break;
-jump_up:
-		if (walker_path.dentry == walker_path.mnt->mnt_root) {
-			if (follow_up(&walker_path)) {
-				/* Ignores hidden mount points. */
-				goto jump_up;
-			} else {
-				/*
-				 * Stops at the real root.  Denies access
-				 * because not all layers have granted access.
-				 */
-				break;
-			}
-		}
+
+		if (path_walk_parent(&walker_path, &root))
+			continue;
+
 		if (unlikely(IS_ROOT(walker_path.dentry))) {
 			/*
-			 * Stops at disconnected root directories.  Only allows
-			 * access to internal filesystems (e.g. nsfs, which is
-			 * reachable through /proc/<pid>/ns/<namespace>).
+			 * Stops at disconnected or real root directories.
+			 * Only allows access to internal filesystems
+			 * (e.g. nsfs, which is reachable through
+			 * /proc/<pid>/ns/<namespace>).
 			 */
 			if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
 				allowed_parent1 = true;
 				allowed_parent2 = true;
 			}
-			break;
 		}
-		parent_dentry = dget_parent(walker_path.dentry);
-		dput(walker_path.dentry);
-		walker_path.dentry = parent_dentry;
+		break;
 	}
 	path_put(&walker_path);
 
-- 
2.47.1

Landlock tests with hostfs fail:

ok 126 layout3_fs.hostfs.tag_inode_file
#  RUN           layout3_fs.hostfs.release_inodes ...
# fs_test.c:5555:release_inodes:Expected EACCES (13) == test_open(TMP_DIR, O_RDONLY) (0)

This specific test checks that an access to a (denied) mount point over
an allowed directory is indeed denied.

It's not clear to me the origin of the issue, but it seems to be related
to choose_mountpoint().

You can run these tests with `check-linux.sh build kselftest` from
https://github.com/landlock-lsm/landlock-test-tools

Just in case, please always run clang-format -i security/landlock/*.[ch]


On Mon, Jun 02, 2025 at 11:59:18PM -0700, Song Liu wrote:
> Use path_walk_parent() to walk a path up to its parent.
> 
> No functional changes intended.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  security/landlock/fs.c | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 6fee7c20f64d..3adac544dc9e 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -837,8 +837,8 @@ static bool is_access_to_paths_allowed(
>  	 * restriction.
>  	 */
>  	while (true) {
> -		struct dentry *parent_dentry;
>  		const struct landlock_rule *rule;
> +		struct path root = {};
>  
>  		/*
>  		 * If at least all accesses allowed on the destination are
> @@ -895,34 +895,23 @@ static bool is_access_to_paths_allowed(
>  		/* Stops when a rule from each layer grants access. */
>  		if (allowed_parent1 && allowed_parent2)
>  			break;
> -jump_up:
> -		if (walker_path.dentry == walker_path.mnt->mnt_root) {
> -			if (follow_up(&walker_path)) {
> -				/* Ignores hidden mount points. */
> -				goto jump_up;
> -			} else {
> -				/*
> -				 * Stops at the real root.  Denies access
> -				 * because not all layers have granted access.
> -				 */
> -				break;
> -			}
> -		}
> +
> +		if (path_walk_parent(&walker_path, &root))
> +			continue;

It would be better to avoid a "continue" statement but to just use an if
block.

> +
>  		if (unlikely(IS_ROOT(walker_path.dentry))) {
>  			/*
> -			 * Stops at disconnected root directories.  Only allows
> -			 * access to internal filesystems (e.g. nsfs, which is
> -			 * reachable through /proc/<pid>/ns/<namespace>).
> +			 * Stops at disconnected or real root directories.
> +			 * Only allows access to internal filesystems
> +			 * (e.g. nsfs, which is reachable through
> +			 * /proc/<pid>/ns/<namespace>).
>  			 */
>  			if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
>  				allowed_parent1 = true;
>  				allowed_parent2 = true;
>  			}
> -			break;
>  		}
> -		parent_dentry = dget_parent(walker_path.dentry);
> -		dput(walker_path.dentry);
> -		walker_path.dentry = parent_dentry;
> +		break;
>  	}
>  	path_put(&walker_path);
>  
> -- 
> 2.47.1
> 
>

On Tue, Jun 3, 2025 at 6:46 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> Landlock tests with hostfs fail:
>
> ok 126 layout3_fs.hostfs.tag_inode_file
> #  RUN           layout3_fs.hostfs.release_inodes ...
> # fs_test.c:5555:release_inodes:Expected EACCES (13) == test_open(TMP_DIR, O_RDONLY) (0)
>
> This specific test checks that an access to a (denied) mount point over
> an allowed directory is indeed denied.

I am having trouble understanding the test. It appears to me
the newly mounted tmpfs on /tmp is allowed, but accesses to
/ and thus mount point /tmp is denied? What would the walk in
is_access_to_paths_allowed look like?

> It's not clear to me the origin of the issue, but it seems to be related
> to choose_mountpoint().
>
> You can run these tests with `check-linux.sh build kselftest` from
> https://github.com/landlock-lsm/landlock-test-tools

How should I debug this test? printk doesn't seem to work.

Thanks,
Song

On Wed, Jun 4, 2025 at 12:37 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, Jun 3, 2025 at 6:46 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Landlock tests with hostfs fail:
> >
> > ok 126 layout3_fs.hostfs.tag_inode_file
> > #  RUN           layout3_fs.hostfs.release_inodes ...
> > # fs_test.c:5555:release_inodes:Expected EACCES (13) == test_open(TMP_DIR, O_RDONLY) (0)
> >
> > This specific test checks that an access to a (denied) mount point over
> > an allowed directory is indeed denied.

I just realized this only fails on hostfs. AFAICT, hostfs is only used
by um. Do we really need this to behave the same on um+hostfs?

Thanks,
Song

>
> I am having trouble understanding the test. It appears to me
> the newly mounted tmpfs on /tmp is allowed, but accesses to
> / and thus mount point /tmp is denied? What would the walk in
> is_access_to_paths_allowed look like?
>
> > It's not clear to me the origin of the issue, but it seems to be related
> > to choose_mountpoint().
> >
> > You can run these tests with `check-linux.sh build kselftest` from
> > https://github.com/landlock-lsm/landlock-test-tools
>
> How should I debug this test? printk doesn't seem to work.
>
> Thanks,
> Song

On Thu, Jun 05, 2025 at 09:47:36AM -0700, Song Liu wrote:
> On Wed, Jun 4, 2025 at 12:37 PM Song Liu <song@kernel.org> wrote:
> >
> > On Tue, Jun 3, 2025 at 6:46 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > Landlock tests with hostfs fail:
> > >
> > > ok 126 layout3_fs.hostfs.tag_inode_file
> > > #  RUN           layout3_fs.hostfs.release_inodes ...
> > > # fs_test.c:5555:release_inodes:Expected EACCES (13) == test_open(TMP_DIR, O_RDONLY) (0)
> > >
> > > This specific test checks that an access to a (denied) mount point over
> > > an allowed directory is indeed denied.
> 
> I just realized this only fails on hostfs. AFAICT, hostfs is only used
> by um. Do we really need this to behave the same on um+hostfs?

Yes, this would be a regression, and in fact it is not related to hostfs
and it would be a new security bug.

The issue is that the path_walk_parent() doesn't return the parent
dentry but the underlying mount point if any.  When choose_mountpoint()
returns true, path_walk_parent() should continue to the following root
check and potentiall the dget_parent() call.  We need to be careful with
the path_put() though.

This issue was only spotted by this hostfs test because this one adds a
rule which is tied to the inode of the mount which is in fact the same
inode of the mount point because the mount is a bind mount.  I'll send a
new test that check the same thing but with tmpfs (for convenience, but
it would be the same for any filesystem).

> 
> Thanks,
> Song
> 
> >
> > I am having trouble understanding the test. It appears to me
> > the newly mounted tmpfs on /tmp is allowed, but accesses to
> > / and thus mount point /tmp is denied? What would the walk in
> > is_access_to_paths_allowed look like?

The test checks that a mount is not wrongly identified as the underlying
mount point.

> >
> > > It's not clear to me the origin of the issue, but it seems to be related
> > > to choose_mountpoint().
> > >
> > > You can run these tests with `check-linux.sh build kselftest` from
> > > https://github.com/landlock-lsm/landlock-test-tools
> >
> > How should I debug this test? printk doesn't seem to work.

The console log level is set to warn, so you can use pr_warn().

> >
> > Thanks,
> > Song