Re: [RFC PATCH v1 1/2] landlock: Fix handling of disconnected directories

[Christian Brauner <brauner@kernel.org>]

On Tue, Jul 01, 2025 at 08:38:07PM +0200, Mickaël Salaün wrote:
> We can get disconnected files or directories when they are visible and
> opened from a bind mount, before being renamed/moved from the source of
> the bind mount in a way that makes them inaccessible from the mount
> point (i.e. out of scope).
> 
> Until now, access rights tied to files or directories opened through a
> disconnected directory were collected by walking the related hierarchy
> down to the root of this filesystem because the mount point couldn't be
> found.  This could lead to inconsistent access results, and
> hard-to-debug renames, especially because such paths cannot be printed.
> 
> For a sandboxed task to create a disconnected directory, it needs to
> have write access (i.e. FS_MAKE_REG, FS_REMOVE_FILE, and FS_REFER) to
> the underlying source of the bind mount, and read access to the related
> mount point.  Because a sandboxed task cannot get more access than those
> defined by its Landlock domain, this could only lead to inconsistent
> access rights because of missing those that should be inherited from the
> mount point hierarchy and inheriting from the hierarchy of the mounted
> filesystem instead.
> 
> Landlock now handles files/directories opened from disconnected
> directories like the mount point these disconnected directories were
> opened from.  This gives the guarantee that access rights on a
> file/directory cannot be more than those at open time.  The rationale is
> that disconnected hierarchies might not be visible nor accessible to a
> sandboxed task, and relying on the collected access rights from them
> could introduce unexpected results, especially for rename actions
> because of the access right comparison between the source and the
> destination (see LANDLOCK_ACCESS_FS_REFER).  This new behavior is much
> less surprising to users and safer from an access point of view.
> 
> Unlike follow_dotdot(), we don't need to check for each directory if it
> is part of the mount's root, but instead this is only checked when we
> reached a root dentry (not a mount point), or when the access
> request is about to be allowed.  This limits the number of calls to
> is_subdir() which walks down the hierarchy (again).  This also avoids
> checking path connection at the beginning of the walk for each mount
> point, which would be racy.
> 
> Make path_connected() public to stay consistent with the VFS.  This
> helper is used when we are about to allowed an access.
> 
> This change increases the stack size with two Landlock layer masks
> backups that are needed to reset the collected access rights to the
> latest mount point.
> 
> Because opened files have their access rights stored in the related file
> security properties, their is no impact for disconnected or unlinked
> files.
> 
> A following commit will document handling of disconnected files and
> directories.
> 
> Cc: Günther Noack <gnoack@google.com>
> Cc: Song Liu <song@kernel.org>
> Reported-by: Tingmao Wang <m@maowtm.org>
> Closes: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org
> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
> Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> This replaces this patch:
> landlock: Remove warning in collect_domain_accesses()
> https://lore.kernel.org/r/20250618134734.1673254-1-mic@digikod.net
> 
> I'll probably split this commit into two to ease backport (same for
> tests).
> 
> This patch series applies on top of my next branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> 
> TODO: Add documentation
> 
> TODO: Add Landlock erratum
> ---
>  fs/namei.c             |   2 +-
>  include/linux/fs.h     |   1 +
>  security/landlock/fs.c | 121 +++++++++++++++++++++++++++++++++++------
>  3 files changed, 105 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 4bb889fc980b..7853a876fc1c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -716,7 +716,7 @@ static bool nd_alloc_stack(struct nameidata *nd)
>   * Rename can sometimes move a file or directory outside of a bind
>   * mount, path_connected allows those cases to be detected.
>   */
> -static bool path_connected(struct vfsmount *mnt, struct dentry *dentry)
> +bool path_connected(struct vfsmount *mnt, struct dentry *dentry)
>  {
>  	struct super_block *sb = mnt->mnt_sb;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 4ec77da65f14..3c0e324a9272 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3252,6 +3252,7 @@ extern struct file * open_exec(const char *);
>  /* fs/dcache.c -- generic fs support functions */
>  extern bool is_subdir(struct dentry *, struct dentry *);
>  extern bool path_is_under(const struct path *, const struct path *);
> +extern bool path_connected(struct vfsmount *mnt, struct dentry *dentry);

Drop the "extern" please. We generally don't do that anymore for new
additions. Otherwise,

Acked-by: Christian Brauner <brauner@kernel.org>