diff mbox series

[RFC] : fhandle: relax open_by_handle_at() permission checks

Message ID 20240524-vfs-open_by_handle_at-v1-1-3d4b7d22736b@kernel.org (mailing list archive)
State New
Headers show
Series [RFC] : fhandle: relax open_by_handle_at() permission checks | expand

Commit Message

Christian Brauner May 24, 2024, 10:19 a.m. UTC 
A current limitation of open_by_handle_at() is that it's currently not possible
to use it from within containers at all because we require CAP_DAC_READ_SEARCH
in the initial namespace. That's unfortunate because there are scenarios where
using open_by_handle_at() from within containers.

Two examples:

(1) cgroupfs allows to encode cgroups to file handles and reopen them with
    open_by_handle_at().
(2) Fanotify allows placing filesystem watches they currently aren't usable in
    containers because the returned file handles cannot be used.

Here's a proposal for relaxing the permission check for open_by_handle_at().

(1) Opening file handles when the caller has privileges over the filesystem
    (1.1) The caller has an unobstructed view of the filesystem.
    (1.2) The caller has permissions to follow a path to the file handle.

This doesn't address the problem of opening a file handle when only a portion
of a filesystem is exposed as is common in containers by e.g., bind-mounting a
subtree. The proposal to solve this use-case is:

(2) Opening file handles when the caller has privileges over a subtree
    (2.1) The caller is able to reach the file from the provided mount fd.
    (2.2) The caller has permissions to construct an unobstructed path to the
          file handle.
    (2.3) The caller has permissions to follow a path to the file handle.

The relaxed permission checks are currently restricted to directory file
handles which are what both cgroupfs and fanotify need. Handling disconnected
non-directory file handles would lead to a potentially non-deterministic api.
If a disconnected non-directory file handle is provided we may fail to decode
a valid path that we could use for permission checking. That in itself isn't a
problem as we would just return EACCES in that case. However, confusion may
arise if a non-disconnected dentry ends up in the cache later and those opening
the file handle would suddenly succeed.

* It's potentially possible to use timing information (side-channel) to infer
  whether a given inode exists. I don't think that's particularly
  problematic. Thanks to Jann for bringing this to my attention.

* An unrelated note (IOW, these are thoughts that apply to
  open_by_handle_at() generically and are unrelated to the changes here):
  Jann pointed out that we should verify whether deleted files could
  potentially be reopened through open_by_handle_at(). I don't think that's
  possible though.

  Another potential thing to check is whether open_by_handle_at() could be
  abused to open internal stuff like memfds or gpu stuff. I don't think so
  but I haven't had the time to completely verify this.

This dates back to discussions Amir and I had quite some time ago and thanks to
him for providing a lot of details around the export code and related patches!

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/exportfs/expfs.c      |   9 ++-
 fs/fhandle.c             | 162 ++++++++++++++++++++++++++++++++++++-----------
 fs/mount.h               |   1 +
 fs/namespace.c           |   2 +-
 fs/nfsd/nfsfh.c          |   2 +-
 include/linux/exportfs.h |   1 +
 6 files changed, 137 insertions(+), 40 deletions(-)


---
base-commit: 8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6
change-id: 20240524-vfs-open_by_handle_at-73c20767eb4e

Comments

Amir Goldstein May 24, 2024, 12:35 p.m. UTC | #1 
On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
>
> A current limitation of open_by_handle_at() is that it's currently not possible
> to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> in the initial namespace. That's unfortunate because there are scenarios where
> using open_by_handle_at() from within containers.
>
> Two examples:
>
> (1) cgroupfs allows to encode cgroups to file handles and reopen them with
>     open_by_handle_at().
> (2) Fanotify allows placing filesystem watches they currently aren't usable in
>     containers because the returned file handles cannot be used.
>
> Here's a proposal for relaxing the permission check for open_by_handle_at().
>
> (1) Opening file handles when the caller has privileges over the filesystem
>     (1.1) The caller has an unobstructed view of the filesystem.
>     (1.2) The caller has permissions to follow a path to the file handle.
>
> This doesn't address the problem of opening a file handle when only a portion
> of a filesystem is exposed as is common in containers by e.g., bind-mounting a
> subtree. The proposal to solve this use-case is:
>
> (2) Opening file handles when the caller has privileges over a subtree
>     (2.1) The caller is able to reach the file from the provided mount fd.
>     (2.2) The caller has permissions to construct an unobstructed path to the
>           file handle.
>     (2.3) The caller has permissions to follow a path to the file handle.
>
> The relaxed permission checks are currently restricted to directory file
> handles which are what both cgroupfs and fanotify need. Handling disconnected
> non-directory file handles would lead to a potentially non-deterministic api.
> If a disconnected non-directory file handle is provided we may fail to decode
> a valid path that we could use for permission checking. That in itself isn't a
> problem as we would just return EACCES in that case. However, confusion may
> arise if a non-disconnected dentry ends up in the cache later and those opening
> the file handle would suddenly succeed.
>
> * It's potentially possible to use timing information (side-channel) to infer
>   whether a given inode exists. I don't think that's particularly
>   problematic. Thanks to Jann for bringing this to my attention.
>
> * An unrelated note (IOW, these are thoughts that apply to
>   open_by_handle_at() generically and are unrelated to the changes here):
>   Jann pointed out that we should verify whether deleted files could
>   potentially be reopened through open_by_handle_at(). I don't think that's
>   possible though.

AFAICT, the only thing that currently makes this impossible in this patch
is the O_DIRECTORY limitation.
Because there could be non-directory non-hashed aliases of deleted
files in dcache.

>
>   Another potential thing to check is whether open_by_handle_at() could be
>   abused to open internal stuff like memfds or gpu stuff. I don't think so
>   but I haven't had the time to completely verify this.

Yeh, I don't think that those fs have ->s_export_op.

>
> This dates back to discussions Amir and I had quite some time ago and thanks to
> him for providing a lot of details around the export code and related patches!
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Apart from minor nits below, you may add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/exportfs/expfs.c      |   9 ++-
>  fs/fhandle.c             | 162 ++++++++++++++++++++++++++++++++++++-----------
>  fs/mount.h               |   1 +
>  fs/namespace.c           |   2 +-
>  fs/nfsd/nfsfh.c          |   2 +-
>  include/linux/exportfs.h |   1 +
>  6 files changed, 137 insertions(+), 40 deletions(-)
>
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 07ea3d62b298..b23b052df715 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -427,7 +427,7 @@ EXPORT_SYMBOL_GPL(exportfs_encode_fh);
>
>  struct dentry *
>  exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
> -                      int fileid_type,
> +                      int fileid_type, bool directory,
>                        int (*acceptable)(void *, struct dentry *),
>                        void *context)
>  {
> @@ -445,6 +445,11 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
>         if (IS_ERR_OR_NULL(result))
>                 return result;
>
> +       if (directory && !d_is_dir(result)) {
> +               err = -ENOTDIR;
> +               goto err_result;
> +       }
> +
>         /*
>          * If no acceptance criteria was specified by caller, a disconnected
>          * dentry is also accepatable. Callers may use this mode to query if
> @@ -581,7 +586,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  {
>         struct dentry *ret;
>
> -       ret = exportfs_decode_fh_raw(mnt, fid, fh_len, fileid_type,
> +       ret = exportfs_decode_fh_raw(mnt, fid, fh_len, fileid_type, false,
>                                      acceptable, context);
>         if (IS_ERR_OR_NULL(ret)) {
>                 if (ret == ERR_PTR(-ENOMEM))
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 8a7f86c2139a..c6ed832ddbb8 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -115,88 +115,174 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>         return err;
>  }
>
> -static struct vfsmount *get_vfsmount_from_fd(int fd)
> +static int get_path_from_fd(int fd, struct path *root)
>  {
> -       struct vfsmount *mnt;
> -
>         if (fd == AT_FDCWD) {
>                 struct fs_struct *fs = current->fs;
>                 spin_lock(&fs->lock);
> -               mnt = mntget(fs->pwd.mnt);
> +               *root = fs->pwd;
> +               path_get(root);
>                 spin_unlock(&fs->lock);
>         } else {
>                 struct fd f = fdget(fd);
>                 if (!f.file)
> -                       return ERR_PTR(-EBADF);
> -               mnt = mntget(f.file->f_path.mnt);
> +                       return -EBADF;
> +               *root = f.file->f_path;
> +               path_get(root);
>                 fdput(f);
>         }
> -       return mnt;
> +
> +       return 0;
>  }
>
> +enum handle_to_path_flags {
> +       HANDLE_CHECK_PERMS   = (1 << 0),
> +       HANDLE_CHECK_SUBTREE = (1 << 1),
> +};
> +
> +struct handle_to_path_ctx {
> +       struct path root;
> +       enum handle_to_path_flags flags;
> +       bool directory;
> +};
> +
>  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  {
> -       return 1;
> +       struct handle_to_path_ctx *ctx = context;
> +       struct user_namespace *user_ns = current_user_ns();
> +       struct dentry *d, *root = ctx->root.dentry;
> +       struct mnt_idmap *idmap = mnt_idmap(ctx->root.mnt);
> +       int retval = 0;
> +
> +       if (!root)
> +               return 1;
> +
> +       /* Old permission model with global CAP_DAC_READ_SEARCH. */
> +       if (!ctx->flags)
> +               return 1;
> +
> +       /*
> +        * It's racy as we're not taking rename_lock but we're able to ignore
> +        * permissions and we just need an approximation whether we were able
> +        * to follow a path to the file.
> +        */
> +       d = dget(dentry);
> +       while (d != root && !IS_ROOT(d)) {
> +               struct dentry *parent = dget_parent(d);
> +
> +               /*
> +                * We know that we have the ability to override DAC permissions
> +                * as we've verified this earlier via CAP_DAC_READ_SEARCH. But
> +                * we also need to make sure that there aren't any unmapped
> +                * inodes in the path that would prevent us from reaching the
> +                * file.
> +                */
> +               if (!privileged_wrt_inode_uidgid(user_ns, idmap,
> +                                                d_inode(parent))) {
> +                       dput(d);
> +                       dput(parent);
> +                       return retval;
> +               }
> +
> +               dput(d);
> +               d = parent;
> +       }
> +
> +       if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> +               retval = 1;
> +

Maybe leave an assertion, to make sure we have not missed
anything with the O_DIRECTORY assumptions:

WARN_ON_ONCE(d == root || d == root->d_sb->s_root);

> +       dput(d);
> +       return retval;
>  }
>
>  static int do_handle_to_path(int mountdirfd, struct file_handle *handle,
> -                            struct path *path)
> +                            struct path *path, struct handle_to_path_ctx *ctx)
>  {
> -       int retval = 0;
>         int handle_dwords;
> +       struct vfsmount *mnt = ctx->root.mnt;
>
> -       path->mnt = get_vfsmount_from_fd(mountdirfd);
> -       if (IS_ERR(path->mnt)) {
> -               retval = PTR_ERR(path->mnt);
> -               goto out_err;
> -       }
>         /* change the handle size to multiple of sizeof(u32) */
>         handle_dwords = handle->handle_bytes >> 2;
> -       path->dentry = exportfs_decode_fh(path->mnt,
> +       path->dentry = exportfs_decode_fh_raw(mnt,
>                                           (struct fid *)handle->f_handle,
>                                           handle_dwords, handle->handle_type,
> -                                         vfs_dentry_acceptable, NULL);
> -       if (IS_ERR(path->dentry)) {
> -               retval = PTR_ERR(path->dentry);
> -               goto out_mnt;
> +                                         ctx->directory,
> +                                         vfs_dentry_acceptable, ctx);
> +       if (IS_ERR_OR_NULL(path->dentry)) {
> +               if (path->dentry == ERR_PTR(-ENOMEM))
> +                       return -ENOMEM;
> +               return -ESTALE;
>         }
> +       path->mnt = mntget(mnt);
>         return 0;
> -out_mnt:
> -       mntput(path->mnt);
> -out_err:
> -       return retval;
>  }
>
>  static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
> -                  struct path *path)
> +                  struct path *path, unsigned int o_flags)
>  {
>         int retval = 0;
>         struct file_handle f_handle;
>         struct file_handle *handle = NULL;
> +       struct handle_to_path_ctx ctx = {};
> +
> +       retval = get_path_from_fd(mountdirfd, &ctx.root);
> +       if (retval)
> +               goto out_err;
>
> -       /*
> -        * With handle we don't look at the execute bit on the
> -        * directory. Ideally we would like CAP_DAC_SEARCH.
> -        * But we don't have that
> -        */
>         if (!capable(CAP_DAC_READ_SEARCH)) {
> +               /*
> +                * Allow relaxed permissions of file handles if the caller has
> +                * the ability to mount the filesystem or create a bind-mount
> +                * of the provided @mountdirfd.
> +                *
> +                * In both cases the caller may be able to get an unobstructed
> +                * way to the encoded file handle. If the caller is only able
> +                * to create a bind-mount we need to verify that there are no
> +                * locked mounts on top of it that could prevent us from
> +                * getting to the encoded file.
> +                *
> +                * In principle, locked mounts can prevent the caller from
> +                * mounting the filesystem but that only applies to procfs and
> +                * sysfs neither of which support decoding file handles.
> +                *
> +                * This is currently restricted to O_DIRECTORY to provide a
> +                * deterministic API that avoids a confusing api in the face of
> +                * disconnected non-dir dentries.
> +                */
> +
>                 retval = -EPERM;
> -               goto out_err;
> +               if (!(o_flags & O_DIRECTORY))
> +                       goto out_path;
> +
> +               if (ns_capable(ctx.root.mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
> +                       ctx.flags = HANDLE_CHECK_PERMS;
> +               else if (ns_capable(real_mount(ctx.root.mnt)->mnt_ns->user_ns, CAP_SYS_ADMIN) &&
> +                          !has_locked_children(real_mount(ctx.root.mnt), ctx.root.dentry))
> +                       ctx.flags = HANDLE_CHECK_PERMS | HANDLE_CHECK_SUBTREE;
> +               else
> +                       goto out_path;
> +
> +               /* Are we able to override DAC permissions? */
> +               if (!ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH))
> +                       goto out_path;
> +
> +               ctx.directory = true;
>         }
> +
>         if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
>                 retval = -EFAULT;
> -               goto out_err;
> +               goto out_path;
>         }
>         if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
>             (f_handle.handle_bytes == 0)) {
>                 retval = -EINVAL;
> -               goto out_err;
> +               goto out_path;
>         }
>         handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
>                          GFP_KERNEL);
>         if (!handle) {
>                 retval = -ENOMEM;
> -               goto out_err;
> +               goto out_path;
>         }
>         /* copy the full handle */
>         *handle = f_handle;
> @@ -207,10 +293,14 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>                 goto out_handle;
>         }
>
> -       retval = do_handle_to_path(mountdirfd, handle, path);
> +       retval = do_handle_to_path(mountdirfd, handle, path, &ctx);
> +       if (retval)
> +               goto out_handle;

no real need for this goto unless you wanted it here for future code.

>
>  out_handle:
>         kfree(handle);
> +out_path:
> +       path_put(&ctx.root);
>  out_err:
>         return retval;
>  }
> @@ -223,7 +313,7 @@ static long do_handle_open(int mountdirfd, struct file_handle __user *ufh,
>         struct file *file;
>         int fd;
>
> -       retval = handle_to_path(mountdirfd, ufh, &path);
> +       retval = handle_to_path(mountdirfd, ufh, &path, open_flag);
>         if (retval)
>                 return retval;
>
> diff --git a/fs/mount.h b/fs/mount.h
> index 4a42fc68f4cc..4adce73211ae 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -152,3 +152,4 @@ static inline void move_from_ns(struct mount *mnt, struct list_head *dt_list)
>  }
>
>  extern void mnt_cursor_del(struct mnt_namespace *ns, struct mount *cursor);
> +bool has_locked_children(struct mount *mnt, struct dentry *dentry);
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5a51315c6678..4386787210c7 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2078,7 +2078,7 @@ void drop_collected_mounts(struct vfsmount *mnt)
>         namespace_unlock();
>  }
>
> -static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
> +bool has_locked_children(struct mount *mnt, struct dentry *dentry)
>  {
>         struct mount *child;
>
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 0b75305fb5f5..3e7f81eb2818 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -247,7 +247,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
>                 dentry = dget(exp->ex_path.dentry);
>         else {
>                 dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid,
> -                                               data_left, fileid_type,
> +                                               data_left, fileid_type, false,
>                                                 nfsd_acceptable, exp);
>                 if (IS_ERR_OR_NULL(dentry)) {
>                         trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index bb37ad5cc954..90c4b0111218 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -305,6 +305,7 @@ static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>                                              struct fid *fid, int fh_len,
>                                              int fileid_type,
> +                                            bool directory,
>                                              int (*acceptable)(void *, struct dentry *),
>                                              void *context);
>  extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,


This was confusing me when I saw callers that pass directory=false
for any decode, until I realized this was only_directory.
But I got used to it already ;)

Thanks,
Amir.
Jeff Layton May 25, 2024, 10:55 a.m. UTC | #2 
On Fri, 2024-05-24 at 12:19 +0200, Christian Brauner wrote:
> A current limitation of open_by_handle_at() is that it's currently not possible
> to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> in the initial namespace. That's unfortunate because there are scenarios where
> using open_by_handle_at() from within containers.
> 
> Two examples:
> 
> (1) cgroupfs allows to encode cgroups to file handles and reopen them with
>     open_by_handle_at().
> (2) Fanotify allows placing filesystem watches they currently aren't usable in
>     containers because the returned file handles cannot be used.
> 
> Here's a proposal for relaxing the permission check for open_by_handle_at().
> 
> (1) Opening file handles when the caller has privileges over the filesystem
>     (1.1) The caller has an unobstructed view of the filesystem.
>     (1.2) The caller has permissions to follow a path to the file handle.
> 
> This doesn't address the problem of opening a file handle when only a portion
> of a filesystem is exposed as is common in containers by e.g., bind-mounting a
> subtree. The proposal to solve this use-case is:
> 
> (2) Opening file handles when the caller has privileges over a subtree
>     (2.1) The caller is able to reach the file from the provided mount fd.
>     (2.2) The caller has permissions to construct an unobstructed path to the
>           file handle.
>     (2.3) The caller has permissions to follow a path to the file handle.
>
> The relaxed permission checks are currently restricted to directory file
> handles which are what both cgroupfs and fanotify need. Handling disconnected
> non-directory file handles would lead to a potentially non-deterministic api.
> If a disconnected non-directory file handle is provided we may fail to decode
> a valid path that we could use for permission checking. That in itself isn't a
> problem as we would just return EACCES in that case. However, confusion may
> arise if a non-disconnected dentry ends up in the cache later and those opening
> the file handle would suddenly succeed.
> 

The rules here seem sane to me, and I support making it simpler for
unprivileged users to figure out filehandles.

> * It's potentially possible to use timing information (side-channel) to infer
>   whether a given inode exists. I don't think that's particularly
>   problematic. Thanks to Jann for bringing this to my attention.
> 
> * An unrelated note (IOW, these are thoughts that apply to
>   open_by_handle_at() generically and are unrelated to the changes here):
>   Jann pointed out that we should verify whether deleted files could
>   potentially be reopened through open_by_handle_at(). I don't think that's
>   possible though.
> 
>   Another potential thing to check is whether open_by_handle_at() could be
>   abused to open internal stuff like memfds or gpu stuff. I don't think so
>   but I haven't had the time to completely verify this.
> 
> This dates back to discussions Amir and I had quite some time ago and thanks to
> him for providing a lot of details around the export code and related patches!
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/exportfs/expfs.c      |   9 ++-
>  fs/fhandle.c             | 162 ++++++++++++++++++++++++++++++++++++-----------
>  fs/mount.h               |   1 +
>  fs/namespace.c           |   2 +-
>  fs/nfsd/nfsfh.c          |   2 +-
>  include/linux/exportfs.h |   1 +
>  6 files changed, 137 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 07ea3d62b298..b23b052df715 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -427,7 +427,7 @@ EXPORT_SYMBOL_GPL(exportfs_encode_fh);
>  
>  struct dentry *
>  exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
> -		       int fileid_type,
> +		       int fileid_type, bool directory,
>  		       int (*acceptable)(void *, struct dentry *),
>  		       void *context)
>  {
> @@ -445,6 +445,11 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
>  	if (IS_ERR_OR_NULL(result))
>  		return result;
>  
> +	if (directory && !d_is_dir(result)) {
> +		err = -ENOTDIR;
> +		goto err_result;
> +	}
> +
>  	/*
>  	 * If no acceptance criteria was specified by caller, a disconnected
>  	 * dentry is also accepatable. Callers may use this mode to query if
> @@ -581,7 +586,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  {
>  	struct dentry *ret;
>  
> -	ret = exportfs_decode_fh_raw(mnt, fid, fh_len, fileid_type,
> +	ret = exportfs_decode_fh_raw(mnt, fid, fh_len, fileid_type, false,
>  				     acceptable, context);
>  	if (IS_ERR_OR_NULL(ret)) {
>  		if (ret == ERR_PTR(-ENOMEM))
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 8a7f86c2139a..c6ed832ddbb8 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -115,88 +115,174 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	return err;
>  }
>  
> -static struct vfsmount *get_vfsmount_from_fd(int fd)
> +static int get_path_from_fd(int fd, struct path *root)

nit: you could return struct path here and just return an ERR_PTR if
there is an error.

>  {
> -	struct vfsmount *mnt;
> -
>  	if (fd == AT_FDCWD) {
>  		struct fs_struct *fs = current->fs;
>  		spin_lock(&fs->lock);
> -		mnt = mntget(fs->pwd.mnt);
> +		*root = fs->pwd;
> +		path_get(root);
>  		spin_unlock(&fs->lock);
>  	} else {
>  		struct fd f = fdget(fd);
>  		if (!f.file)
> -			return ERR_PTR(-EBADF);
> -		mnt = mntget(f.file->f_path.mnt);
> +			return -EBADF;
> +		*root = f.file->f_path;
> +		path_get(root);
>  		fdput(f);
>  	}
> -	return mnt;
> +
> +	return 0;
>  }
>  
> +enum handle_to_path_flags {
> +	HANDLE_CHECK_PERMS   = (1 << 0),
> +	HANDLE_CHECK_SUBTREE = (1 << 1),
> +};
> +
> +struct handle_to_path_ctx {
> +	struct path root;
> +	enum handle_to_path_flags flags;
> +	bool directory;
> +};
> +
>  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  {
> -	return 1;
> +	struct handle_to_path_ctx *ctx = context;
> +	struct user_namespace *user_ns = current_user_ns();
> +	struct dentry *d, *root = ctx->root.dentry;
> +	struct mnt_idmap *idmap = mnt_idmap(ctx->root.mnt);
> +	int retval = 0;
> +
> +	if (!root)
> +		return 1;
> +
> +	/* Old permission model with global CAP_DAC_READ_SEARCH. */
> +	if (!ctx->flags)
> +		return 1;
> +
> +	/*
> +	 * It's racy as we're not taking rename_lock but we're able to ignore
> +	 * permissions and we just need an approximation whether we were able
> +	 * to follow a path to the file.
> +	 */
> +	d = dget(dentry);
> +	while (d != root && !IS_ROOT(d)) {
> +		struct dentry *parent = dget_parent(d);
> +
> +		/*
> +		 * We know that we have the ability to override DAC permissions
> +		 * as we've verified this earlier via CAP_DAC_READ_SEARCH. But
> +		 * we also need to make sure that there aren't any unmapped
> +		 * inodes in the path that would prevent us from reaching the
> +		 * file.
> +		 */
> +		if (!privileged_wrt_inode_uidgid(user_ns, idmap,
> +						 d_inode(parent))) {
> +			dput(d);
> +			dput(parent);
> +			return retval;
> +		}
> +
> +		dput(d);
> +		d = parent;
> +	}

Note that the above will be quite expensive on some filesystems,
particularly if there is a deep path. We've done a similar sort of
checking for a long time in NFS-land, and we really try to avoid it
when possible. Of course there, we do have to also check that the
relevant dentry is exported, which is a bit more costly.

> +
> +	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> +		retval = 1;
> +
> +	dput(d);
> +	return retval;
>  }
>  
>  static int do_handle_to_path(int mountdirfd, struct file_handle *handle,
> -			     struct path *path)
> +			     struct path *path, struct handle_to_path_ctx *ctx)
>  {
> -	int retval = 0;
>  	int handle_dwords;
> +	struct vfsmount *mnt = ctx->root.mnt;
>  
> -	path->mnt = get_vfsmount_from_fd(mountdirfd);
> -	if (IS_ERR(path->mnt)) {
> -		retval = PTR_ERR(path->mnt);
> -		goto out_err;
> -	}
>  	/* change the handle size to multiple of sizeof(u32) */
>  	handle_dwords = handle->handle_bytes >> 2;
> -	path->dentry = exportfs_decode_fh(path->mnt,
> +	path->dentry = exportfs_decode_fh_raw(mnt,
>  					  (struct fid *)handle->f_handle,
>  					  handle_dwords, handle->handle_type,
> -					  vfs_dentry_acceptable, NULL);
> -	if (IS_ERR(path->dentry)) {
> -		retval = PTR_ERR(path->dentry);
> -		goto out_mnt;
> +					  ctx->directory,
> +					  vfs_dentry_acceptable, ctx);
> +	if (IS_ERR_OR_NULL(path->dentry)) {
> +		if (path->dentry == ERR_PTR(-ENOMEM))
> +			return -ENOMEM;
> +		return -ESTALE;
>  	}
> +	path->mnt = mntget(mnt);
>  	return 0;
> -out_mnt:
> -	mntput(path->mnt);
> -out_err:
> -	return retval;
>  }
>  
>  static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
> -		   struct path *path)
> +		   struct path *path, unsigned int o_flags)
>  {
>  	int retval = 0;
>  	struct file_handle f_handle;
>  	struct file_handle *handle = NULL;
> +	struct handle_to_path_ctx ctx = {};
> +
> +	retval = get_path_from_fd(mountdirfd, &ctx.root);
> +	if (retval)
> +		goto out_err;
>  
> -	/*
> -	 * With handle we don't look at the execute bit on the
> -	 * directory. Ideally we would like CAP_DAC_SEARCH.
> -	 * But we don't have that
> -	 */
>  	if (!capable(CAP_DAC_READ_SEARCH)) {
> +		/*
> +		 * Allow relaxed permissions of file handles if the caller has
> +		 * the ability to mount the filesystem or create a bind-mount
> +		 * of the provided @mountdirfd.
> +		 *
> +		 * In both cases the caller may be able to get an unobstructed
> +		 * way to the encoded file handle. If the caller is only able
> +		 * to create a bind-mount we need to verify that there are no
> +		 * locked mounts on top of it that could prevent us from
> +		 * getting to the encoded file.
> +		 *
> +		 * In principle, locked mounts can prevent the caller from
> +		 * mounting the filesystem but that only applies to procfs and
> +		 * sysfs neither of which support decoding file handles.
> +		 *
> +		 * This is currently restricted to O_DIRECTORY to provide a
> +		 * deterministic API that avoids a confusing api in the face of
> +		 * disconnected non-dir dentries.
> +		 */
> +
>  		retval = -EPERM;
> -		goto out_err;
> +		if (!(o_flags & O_DIRECTORY))
> +			goto out_path;
> +
> +		if (ns_capable(ctx.root.mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
> +			ctx.flags = HANDLE_CHECK_PERMS;
> +		else if (ns_capable(real_mount(ctx.root.mnt)->mnt_ns->user_ns, CAP_SYS_ADMIN) &&
> +			   !has_locked_children(real_mount(ctx.root.mnt), ctx.root.dentry))
> +			ctx.flags = HANDLE_CHECK_PERMS | HANDLE_CHECK_SUBTREE;
> +		else
> +			goto out_path;
> +
> +		/* Are we able to override DAC permissions? */
> +		if (!ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH))
> +			goto out_path;
> +
> +		ctx.directory = true;
>  	}
> +
>  	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
>  		retval = -EFAULT;
> -		goto out_err;
> +		goto out_path;
>  	}
>  	if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
>  	    (f_handle.handle_bytes == 0)) {
>  		retval = -EINVAL;
> -		goto out_err;
> +		goto out_path;
>  	}
>  	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
>  			 GFP_KERNEL);
>  	if (!handle) {
>  		retval = -ENOMEM;
> -		goto out_err;
> +		goto out_path;
>  	}
>  	/* copy the full handle */
>  	*handle = f_handle;
> @@ -207,10 +293,14 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		goto out_handle;
>  	}
>  
> -	retval = do_handle_to_path(mountdirfd, handle, path);
> +	retval = do_handle_to_path(mountdirfd, handle, path, &ctx);
> +	if (retval)
> +		goto out_handle;
>  
>  out_handle:
>  	kfree(handle);
> +out_path:
> +	path_put(&ctx.root);
>  out_err:
>  	return retval;
>  }
> @@ -223,7 +313,7 @@ static long do_handle_open(int mountdirfd, struct file_handle __user *ufh,
>  	struct file *file;
>  	int fd;
>  
> -	retval = handle_to_path(mountdirfd, ufh, &path);
> +	retval = handle_to_path(mountdirfd, ufh, &path, open_flag);
>  	if (retval)
>  		return retval;
>  
> diff --git a/fs/mount.h b/fs/mount.h
> index 4a42fc68f4cc..4adce73211ae 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -152,3 +152,4 @@ static inline void move_from_ns(struct mount *mnt, struct list_head *dt_list)
>  }
>  
>  extern void mnt_cursor_del(struct mnt_namespace *ns, struct mount *cursor);
> +bool has_locked_children(struct mount *mnt, struct dentry *dentry);
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5a51315c6678..4386787210c7 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2078,7 +2078,7 @@ void drop_collected_mounts(struct vfsmount *mnt)
>  	namespace_unlock();
>  }
>  
> -static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
> +bool has_locked_children(struct mount *mnt, struct dentry *dentry)
>  {
>  	struct mount *child;
>  
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 0b75305fb5f5..3e7f81eb2818 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -247,7 +247,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
>  		dentry = dget(exp->ex_path.dentry);
>  	else {
>  		dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid,
> -						data_left, fileid_type,
> +						data_left, fileid_type, false,
>  						nfsd_acceptable, exp);
>  		if (IS_ERR_OR_NULL(dentry)) {
>  			trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index bb37ad5cc954..90c4b0111218 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -305,6 +305,7 @@ static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>  					     struct fid *fid, int fh_len,
>  					     int fileid_type,
> +					     bool directory,
>  					     int (*acceptable)(void *, struct dentry *),
>  					     void *context);
>  extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
> 
> ---
> base-commit: 8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6
> change-id: 20240524-vfs-open_by_handle_at-73c20767eb4e
> 

Otherwise, this seems rather sane. Let's see what breaks!

Reviewed-by: Jeff Layton <jlayton@kernel.org>
kernel test robot May 27, 2024, 2:49 a.m. UTC | #3 
Hello,

kernel test robot noticed "BUG:kernel_NULL_pointer_dereference,address" on:

commit: 9ca8b65e411ba759831af5d678f8d01e141816a1 ("[PATCH RFC] : fhandle: relax open_by_handle_at() permission checks")
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Brauner/fhandle-relax-open_by_handle_at-permission-checks/20240524-182059
patch link: https://lore.kernel.org/all/20240524-vfs-open_by_handle_at-v1-1-3d4b7d22736b@kernel.org/
patch subject: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks

in testcase: trinity
version: 
with following parameters:

	runtime: 600s



compiler: gcc-13
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+---------------------------------------------+------------+------------+
|                                             | 8f6a15f095 | 9ca8b65e41 |
+---------------------------------------------+------------+------------+
| boot_successes                              | 4          | 0          |
| boot_failures                               | 0          | 6          |
| BUG:kernel_NULL_pointer_dereference,address | 0          | 6          |
| Oops:Oops:#[##]                             | 0          | 6          |
| EIP:handle_to_path                          | 0          | 6          |
| Kernel_panic-not_syncing:Fatal_exception    | 0          | 6          |
+---------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202405271007.7e95eb21-lkp@intel.com


[   20.927410][  T678] BUG: kernel NULL pointer dereference, address: 00000002
[   20.928271][  T678] #PF: supervisor read access in kernel mode
[   20.928887][  T678] #PF: error_code(0x0000) - not-present page
[   20.929607][  T678] *pde = 00000000
[   20.930090][  T678] Oops: Oops: 0000 [#1]
[   20.930616][  T678] CPU: 0 PID: 678 Comm: trinity-c0 Not tainted 6.9.0-10324-g9ca8b65e411b #1
[ 20.931662][ T678] EIP: handle_to_path (fs/fhandle.c:259 (discriminator 1)) 
[ 20.932243][ T678] Code: f2 ff ff ff e9 95 fe ff ff 8d b6 00 00 00 00 bb ea ff ff ff e9 85 fe ff ff 8d b6 00 00 00 00 8b 45 d8 ba 15 00 00 00 8b 40 6c <8b> 40 18 e8 c1 3a de ff 84 c0 0f 84 5f fe ff ff 8b 45 d8 8b 55 dc
All code
========
   0:	f2 ff                	repnz (bad)
   2:	ff                   	(bad)
   3:	ff                   	(bad)
   4:	e9 95 fe ff ff       	jmp    0xfffffffffffffe9e
   9:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
   f:	bb ea ff ff ff       	mov    $0xffffffea,%ebx
  14:	e9 85 fe ff ff       	jmp    0xfffffffffffffe9e
  19:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
  1f:	8b 45 d8             	mov    -0x28(%rbp),%eax
  22:	ba 15 00 00 00       	mov    $0x15,%edx
  27:	8b 40 6c             	mov    0x6c(%rax),%eax
  2a:*	8b 40 18             	mov    0x18(%rax),%eax		<-- trapping instruction
  2d:	e8 c1 3a de ff       	call   0xffffffffffde3af3
  32:	84 c0                	test   %al,%al
  34:	0f 84 5f fe ff ff    	je     0xfffffffffffffe99
  3a:	8b 45 d8             	mov    -0x28(%rbp),%eax
  3d:	8b 55 dc             	mov    -0x24(%rbp),%edx

Code starting with the faulting instruction
===========================================
   0:	8b 40 18             	mov    0x18(%rax),%eax
   3:	e8 c1 3a de ff       	call   0xffffffffffde3ac9
   8:	84 c0                	test   %al,%al
   a:	0f 84 5f fe ff ff    	je     0xfffffffffffffe6f
  10:	8b 45 d8             	mov    -0x28(%rbp),%eax
  13:	8b 55 dc             	mov    -0x24(%rbp),%edx
[   20.934542][  T678] EAX: ffffffea EBX: c38458c0 ECX: 00000015 EDX: 00000015
[   20.935354][  T678] ESI: ede5bf48 EDI: 00000000 EBP: ede5bf70 ESP: ede5bf2c
[   20.936199][  T678] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 EFLAGS: 00010246
[   20.937022][  T678] CR0: 80050033 CR2: 00000002 CR3: 0370d000 CR4: 00040690
[   20.937713][  T678] Call Trace:
[ 20.938034][ T678] ? show_regs (arch/x86/kernel/dumpstack.c:479) 
[ 20.938520][ T678] ? __die (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:434) 
[ 20.938942][ T678] ? debug_locks_off (lib/debug_locks.c:44) 
[ 20.939502][ T678] ? page_fault_oops (arch/x86/mm/fault.c:715) 
[ 20.940033][ T678] ? kernelmode_fixup_or_oops+0x5c/0x70 
[ 20.940759][ T678] ? __bad_area_nosemaphore+0x113/0x1b4 
[ 20.941504][ T678] ? lock_release (kernel/locking/lockdep.c:467 (discriminator 4) kernel/locking/lockdep.c:5776 (discriminator 4)) 
[ 20.942005][ T678] ? up_read (kernel/locking/rwsem.c:1623) 
[ 20.942838][ T678] ? bad_area_nosemaphore (arch/x86/mm/fault.c:835) 
[ 20.943483][ T678] ? do_user_addr_fault (arch/x86/mm/fault.c:1452) 
[ 20.944138][ T678] ? exc_page_fault (arch/x86/include/asm/irqflags.h:26 arch/x86/include/asm/irqflags.h:67 arch/x86/include/asm/irqflags.h:127 arch/x86/mm/fault.c:1489 arch/x86/mm/fault.c:1539) 
[ 20.944774][ T678] ? pvclock_clocksource_read_nowd (arch/x86/mm/fault.c:1494) 
[ 20.945558][ T678] ? handle_exception (arch/x86/entry/entry_32.S:1054) 
[ 20.946219][ T678] ? keyring_search_rcu (include/linux/refcount.h:192 include/linux/refcount.h:241 include/linux/refcount.h:258 include/linux/key.h:308 security/keys/keyring.c:923) 
[ 20.946845][ T678] ? pvclock_clocksource_read_nowd (arch/x86/mm/fault.c:1494) 
[ 20.947517][ T678] ? handle_to_path (fs/fhandle.c:259 (discriminator 1)) 
[ 20.948115][ T678] ? pvclock_clocksource_read_nowd (arch/x86/mm/fault.c:1494) 
[ 20.948896][ T678] ? handle_to_path (fs/fhandle.c:259 (discriminator 1)) 
[ 20.949505][ T678] ? __lock_release+0x54/0x170 
[ 20.950147][ T678] ? __task_pid_nr_ns (include/linux/rcupdate.h:810 kernel/pid.c:514) 
[ 20.950699][ T678] __ia32_sys_open_by_handle_at (fs/fhandle.c:317 fs/fhandle.c:357 fs/fhandle.c:348 fs/fhandle.c:348) 
[ 20.951279][ T678] ? syscall_exit_to_user_mode (kernel/entry/common.c:221) 
[ 20.951859][ T678] ia32_sys_call (arch/x86/entry/syscall_32.c:42) 
[ 20.952409][ T678] do_int80_syscall_32 (arch/x86/entry/common.c:165 (discriminator 1) arch/x86/entry/common.c:339 (discriminator 1)) 
[ 20.953037][ T678] entry_INT80_32 (arch/x86/entry/entry_32.S:944) 
[   20.953604][  T678] EIP: 0x8097522
[ 20.954040][ T678] Code: 89 c8 c3 90 8d 74 26 00 85 c0 c7 01 01 00 00 00 75 d8 a1 cc 3c ad 08 eb d1 66 90 66 90 66 90 66 90 66 90 66 90 66 90 90 cd 80 <c3> 8d b6 00 00 00 00 8d bc 27 00 00 00 00 8b 10 a3 f4 3c ad 08 85
All code
========
   0:	89 c8                	mov    %ecx,%eax
   2:	c3                   	ret
   3:	90                   	nop
   4:	8d 74 26 00          	lea    0x0(%rsi,%riz,1),%esi
   8:	85 c0                	test   %eax,%eax
   a:	c7 01 01 00 00 00    	movl   $0x1,(%rcx)
  10:	75 d8                	jne    0xffffffffffffffea
  12:	a1 cc 3c ad 08 eb d1 	movabs 0x9066d1eb08ad3ccc,%eax
  19:	66 90 
  1b:	66 90                	xchg   %ax,%ax
  1d:	66 90                	xchg   %ax,%ax
  1f:	66 90                	xchg   %ax,%ax
  21:	66 90                	xchg   %ax,%ax
  23:	66 90                	xchg   %ax,%ax
  25:	66 90                	xchg   %ax,%ax
  27:	90                   	nop
  28:	cd 80                	int    $0x80
  2a:*	c3                   	ret		<-- trapping instruction
  2b:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
  31:	8d bc 27 00 00 00 00 	lea    0x0(%rdi,%riz,1),%edi
  38:	8b 10                	mov    (%rax),%edx
  3a:	a3                   	.byte 0xa3
  3b:	f4                   	hlt
  3c:	3c ad                	cmp    $0xad,%al
  3e:	08                   	.byte 0x8
  3f:	85                   	.byte 0x85

Code starting with the faulting instruction
===========================================
   0:	c3                   	ret
   1:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
   7:	8d bc 27 00 00 00 00 	lea    0x0(%rdi,%riz,1),%edi
   e:	8b 10                	mov    (%rax),%edx
  10:	a3                   	.byte 0xa3
  11:	f4                   	hlt
  12:	3c ad                	cmp    $0xad,%al
  14:	08                   	.byte 0x8
  15:	85                   	.byte 0x85
[   20.956462][  T678] EAX: ffffffda EBX: 00000136 ECX: 00000001 EDX: 00033f01
[   20.957337][  T678] ESI: 000001b6 EDI: fffffff9 EBP: fffffff8 ESP: bf997c98
[   20.958254][  T678] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000296
[   20.959207][  T678] Modules linked in:
[   20.959695][  T678] CR2: 0000000000000002
[   20.960372][  T678] ---[ end trace 0000000000000000 ]---
[ 20.960979][ T678] EIP: handle_to_path (fs/fhandle.c:259 (discriminator 1)) 
[ 20.961566][ T678] Code: f2 ff ff ff e9 95 fe ff ff 8d b6 00 00 00 00 bb ea ff ff ff e9 85 fe ff ff 8d b6 00 00 00 00 8b 45 d8 ba 15 00 00 00 8b 40 6c <8b> 40 18 e8 c1 3a de ff 84 c0 0f 84 5f fe ff ff 8b 45 d8 8b 55 dc
All code
========
   0:	f2 ff                	repnz (bad)
   2:	ff                   	(bad)
   3:	ff                   	(bad)
   4:	e9 95 fe ff ff       	jmp    0xfffffffffffffe9e
   9:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
   f:	bb ea ff ff ff       	mov    $0xffffffea,%ebx
  14:	e9 85 fe ff ff       	jmp    0xfffffffffffffe9e
  19:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
  1f:	8b 45 d8             	mov    -0x28(%rbp),%eax
  22:	ba 15 00 00 00       	mov    $0x15,%edx
  27:	8b 40 6c             	mov    0x6c(%rax),%eax
  2a:*	8b 40 18             	mov    0x18(%rax),%eax		<-- trapping instruction
  2d:	e8 c1 3a de ff       	call   0xffffffffffde3af3
  32:	84 c0                	test   %al,%al
  34:	0f 84 5f fe ff ff    	je     0xfffffffffffffe99
  3a:	8b 45 d8             	mov    -0x28(%rbp),%eax
  3d:	8b 55 dc             	mov    -0x24(%rbp),%edx

Code starting with the faulting instruction
===========================================
   0:	8b 40 18             	mov    0x18(%rax),%eax
   3:	e8 c1 3a de ff       	call   0xffffffffffde3ac9
   8:	84 c0                	test   %al,%al
   a:	0f 84 5f fe ff ff    	je     0xfffffffffffffe6f
  10:	8b 45 d8             	mov    -0x28(%rbp),%eax
  13:	8b 55 dc             	mov    -0x24(%rbp),%edx


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240527/202405271007.7e95eb21-lkp@intel.com
Christoph Hellwig May 27, 2024, 11:31 a.m. UTC | #4 
On Fri, May 24, 2024 at 12:19:39PM +0200, Christian Brauner wrote:
> (2) Opening file handles when the caller has privileges over a subtree
>     (2.1) The caller is able to reach the file from the provided mount fd.
>     (2.2) The caller has permissions to construct an unobstructed path to the
>           file handle.
>     (2.3) The caller has permissions to follow a path to the file handle.

These are OR conditions, not AND, right?

> The relaxed permission checks are currently restricted to directory file
> handles which are what both cgroupfs and fanotify need. Handling disconnected
> non-directory file handles would lead to a potentially non-deterministic api.
> If a disconnected non-directory file handle is provided we may fail to decode
> a valid path that we could use for permission checking. That in itself isn't a
> problem as we would just return EACCES in that case. However, confusion may
> arise if a non-disconnected dentry ends up in the cache later and those opening
> the file handle would suddenly succeed.

That feels like a pretty odd adhoc API.

> * An unrelated note (IOW, these are thoughts that apply to
>   open_by_handle_at() generically and are unrelated to the changes here):
>   Jann pointed out that we should verify whether deleted files could
>   potentially be reopened through open_by_handle_at(). I don't think that's
>   possible though.

What do you mean with "deleted"?  If it is open but unlinked, yes open
by handle allows to get a referene to them.  If it is unlinked and
evicted open by handle should not allow to open it, but it's up to the
file systems to enforce this.

>  struct dentry *
>  exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
> -		       int fileid_type,
> +		       int fileid_type, bool directory,

This is a reall a only_directories flag, right?  Maybe spell that out,
and preferably do that as a flag in a flags paramter so that it also
is obvious in the callers, which a plain true/false is not.

> +struct handle_to_path_ctx {
> +	struct path root;
> +	enum handle_to_path_flags flags;
> +	bool directory;

This and the bool directory passed in a few places 

> +};
> +


> -	path->dentry = exportfs_decode_fh(path->mnt,
> +	path->dentry = exportfs_decode_fh_raw(mnt,

Given that plain exportfs_decode_fh calles are basically dying out
can we just kill it and move the errno gymnastics into the callers
instead of the exportfs_decode_fh vs exportfs_decode_fh_raw
confusion (I still don't understand what's raw about it..)

>  	if (!capable(CAP_DAC_READ_SEARCH)) {
> +		/*
> +		 * Allow relaxed permissions of file handles if the caller has
> +		 * the ability to mount the filesystem or create a bind-mount
> +		 * of the provided @mountdirfd.
> +		 *
> +		 * In both cases the caller may be able to get an unobstructed
> +		 * way to the encoded file handle. If the caller is only able
> +		 * to create a bind-mount we need to verify that there are no
> +		 * locked mounts on top of it that could prevent us from
> +		 * getting to the encoded file.
> +		 *
> +		 * In principle, locked mounts can prevent the caller from
> +		 * mounting the filesystem but that only applies to procfs and
> +		 * sysfs neither of which support decoding file handles.
> +		 *
> +		 * This is currently restricted to O_DIRECTORY to provide a
> +		 * deterministic API that avoids a confusing api in the face of
> +		 * disconnected non-dir dentries.
> +		 */
> +
>  		retval = -EPERM;
> -		goto out_err;
> +		if (!(o_flags & O_DIRECTORY))
> +			goto out_path;
> +
> +		if (ns_capable(ctx.root.mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
> +			ctx.flags = HANDLE_CHECK_PERMS;
> +		else if (ns_capable(real_mount(ctx.root.mnt)->mnt_ns->user_ns, CAP_SYS_ADMIN) &&
> +			   !has_locked_children(real_mount(ctx.root.mnt), ctx.root.dentry))
> +			ctx.flags = HANDLE_CHECK_PERMS | HANDLE_CHECK_SUBTREE;
> +		else
> +			goto out_path;
> +
> +		/* Are we able to override DAC permissions? */
> +		if (!ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH))
> +			goto out_path;
> +
> +		ctx.directory = true;

Can you split this into a separate helper to keep it readable?

And maybe add a comment oon the real_mount because as-is I don't really
understand it at all.
Amir Goldstein Oct. 13, 2024, 4:34 p.m. UTC | #5 
On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > A current limitation of open_by_handle_at() is that it's currently not possible
> > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > in the initial namespace. That's unfortunate because there are scenarios where
> > using open_by_handle_at() from within containers.
> >
> > Two examples:
> >
> > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> >     open_by_handle_at().
> > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> >     containers because the returned file handles cannot be used.
> >

Christian,

Follow up question:
Now that open_by_handle_at(2) is supported from non-root userns,
What about this old patch to allow sb/mount watches from non-root userns?
https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/

Is it useful for any of your use cases?
Should I push it forward?

I have rebased and sanity tested it:
https://github.com/amir73il/linux/commits/fanotify_userns/

But I have not written any userns fanotify test yet.
If you are interested, please let me know.
If you can test the patch for your use case even better.

BTW, the prep patch that I refactored per Jan's review request
is almost identical to your patch from 2019:
https://lore.kernel.org/linux-fsdevel/20190522163150.16849-1-christian@brauner.io/

But with additional BUILD_BUG_ON and a commit message to explain it.

> > Here's a proposal for relaxing the permission check for open_by_handle_at().
> >
> > (1) Opening file handles when the caller has privileges over the filesystem
> >     (1.1) The caller has an unobstructed view of the filesystem.
> >     (1.2) The caller has permissions to follow a path to the file handle.
> >
> > This doesn't address the problem of opening a file handle when only a portion
> > of a filesystem is exposed as is common in containers by e.g., bind-mounting a
> > subtree. The proposal to solve this use-case is:
> >
> > (2) Opening file handles when the caller has privileges over a subtree
> >     (2.1) The caller is able to reach the file from the provided mount fd.
> >     (2.2) The caller has permissions to construct an unobstructed path to the
> >           file handle.
> >     (2.3) The caller has permissions to follow a path to the file handle.
> >
> > The relaxed permission checks are currently restricted to directory file
> > handles which are what both cgroupfs and fanotify need. Handling disconnected
> > non-directory file handles would lead to a potentially non-deterministic api.
> > If a disconnected non-directory file handle is provided we may fail to decode
> > a valid path that we could use for permission checking. That in itself isn't a
> > problem as we would just return EACCES in that case. However, confusion may
> > arise if a non-disconnected dentry ends up in the cache later and those opening
> > the file handle would suddenly succeed.
> >
> > * It's potentially possible to use timing information (side-channel) to infer
> >   whether a given inode exists. I don't think that's particularly
> >   problematic. Thanks to Jann for bringing this to my attention.
> >
> > * An unrelated note (IOW, these are thoughts that apply to
> >   open_by_handle_at() generically and are unrelated to the changes here):
> >   Jann pointed out that we should verify whether deleted files could
> >   potentially be reopened through open_by_handle_at(). I don't think that's
> >   possible though.
>
> AFAICT, the only thing that currently makes this impossible in this patch
> is the O_DIRECTORY limitation.
> Because there could be non-directory non-hashed aliases of deleted
> files in dcache.
>

This limitation, that you added to my request, has implications on
fanotify (TLDR):

file handles reported in fanotify events extra info of type
FAN_EVENT_INFO_TYPE_DFID{,_NAME} due to fanotify_inide()
flag FAN_REPORT_DIR_FID are eligible for open_by_handle_at(2)
inside non-root userns and FAN_REPORT_DFID_NAME is indeed
the most expected use case - namely
directory file handle can always be used to find the parent's path
and the name of the child entry is provided in the event.

HOWEVER,
with fanotify_init() flags FAN_REPORT_{TARGET_,}FID
the child file handle is reported additionally, using extra info type
FAN_EVENT_INFO_TYPE_FID and this file handle may not be
eligible for open_by_handle_at(2) inside non-root userns,
because it could be resolved to a disconnected dentry, for which
permission cannot be checked.

I don't think this is a problem - the child fid is supposed to
be used as an extra check to verify with name_to_handle_at()
that the parent dir and child name refer to the same object as
the one reported with the child fid.
But it can be a bit confusing to users that some file handles
(FAN_EVENT_INFO_TYPE_DFID) can be decoded and some
(FAN_EVENT_INFO_TYPE_FID) cannot, so this hairy detail
will need to be documented.

In retrospect, for unpriv fanotify, we should not have allowed
the basic mode FAN_REPORT_FID, which does not make a
distinction between directory and non-dir file handles.

Jan, do you think we should deny the FAN_REPORT_FID mode
with non-root userns sb/mount watches to reduce some unneeded
rows in the test matrix?

Any other thoughts regarding non-root userns sb/mount watches?

Thanks,
Amir.
Jan Kara Oct. 14, 2024, 4:06 p.m. UTC | #6 
On Sun 13-10-24 18:34:18, Amir Goldstein wrote:
> On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > AFAICT, the only thing that currently makes this impossible in this patch
> > is the O_DIRECTORY limitation.
> > Because there could be non-directory non-hashed aliases of deleted
> > files in dcache.
> >
> 
> This limitation, that you added to my request, has implications on
> fanotify (TLDR):
> 
> file handles reported in fanotify events extra info of type
> FAN_EVENT_INFO_TYPE_DFID{,_NAME} due to fanotify_inide()
> flag FAN_REPORT_DIR_FID are eligible for open_by_handle_at(2)
> inside non-root userns and FAN_REPORT_DFID_NAME is indeed
> the most expected use case - namely
> directory file handle can always be used to find the parent's path
> and the name of the child entry is provided in the event.
> 
> HOWEVER,
> with fanotify_init() flags FAN_REPORT_{TARGET_,}FID
> the child file handle is reported additionally, using extra info type
> FAN_EVENT_INFO_TYPE_FID and this file handle may not be
> eligible for open_by_handle_at(2) inside non-root userns,
> because it could be resolved to a disconnected dentry, for which
> permission cannot be checked.
> 
> I don't think this is a problem - the child fid is supposed to
> be used as an extra check to verify with name_to_handle_at()
> that the parent dir and child name refer to the same object as
> the one reported with the child fid.
> But it can be a bit confusing to users that some file handles
> (FAN_EVENT_INFO_TYPE_DFID) can be decoded and some
> (FAN_EVENT_INFO_TYPE_FID) cannot, so this hairy detail
> will need to be documented.

Well, I think the explanation that open_by_handle_at() works only for
directories inside containers is clear enough explanation. It has nothing
to do with whether the FID comes from FAN_EVENT_INFO_TYPE_DFID or from
FAN_EVENT_INFO_TYPE_FID.

> In retrospect, for unpriv fanotify, we should not have allowed
> the basic mode FAN_REPORT_FID, which does not make a
> distinction between directory and non-dir file handles.

I don't think we could have forseen these changes. Also I don't think the
interface is wrong per se. Just the handle cannot be used with
open_by_handle_at() if it points to a regular file so it may be annoying to
use but we didn't expect the handle will ever be usable with
open_by_handle_at() for unpriviledged users so making it usable for
directories can be seen as an improvement :). Realistically I think
unpriviledged users will use FAN_EVENT_INFO_TYPE_DFID_NAME to avoid these
headaches.
 
> Jan, do you think we should deny the FAN_REPORT_FID mode
> with non-root userns sb/mount watches to reduce some unneeded
> rows in the test matrix?

I think this would be a bit arbitrary restriction. I don't see how
supporting FAN_REPORT_FID would be making our life any harder.

> Any other thoughts regarding non-root userns sb/mount watches?

No, not really.

								Honza
Christian Brauner Oct. 15, 2024, 2:01 p.m. UTC | #7 
On Sun, Oct 13, 2024 at 06:34:18PM +0200, Amir Goldstein wrote:
> On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > A current limitation of open_by_handle_at() is that it's currently not possible
> > > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > > in the initial namespace. That's unfortunate because there are scenarios where
> > > using open_by_handle_at() from within containers.
> > >
> > > Two examples:
> > >
> > > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > >     open_by_handle_at().
> > > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > >     containers because the returned file handles cannot be used.
> > >
> 
> Christian,
> 
> Follow up question:
> Now that open_by_handle_at(2) is supported from non-root userns,
> What about this old patch to allow sb/mount watches from non-root userns?
> https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
> 
> Is it useful for any of your use cases?
> Should I push it forward?

Dammit, I answered that message already yesterday but somehow it didn't
get sent or lost in some other way.

I personally don't have a use-case for it but the systemd folks might
and it would be best to just rope them in.
Amir Goldstein Oct. 16, 2024, 12:45 p.m. UTC | #8 
On Tue, Oct 15, 2024 at 4:01 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sun, Oct 13, 2024 at 06:34:18PM +0200, Amir Goldstein wrote:
> > On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > A current limitation of open_by_handle_at() is that it's currently not possible
> > > > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > > > in the initial namespace. That's unfortunate because there are scenarios where
> > > > using open_by_handle_at() from within containers.
> > > >
> > > > Two examples:
> > > >
> > > > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > > >     open_by_handle_at().
> > > > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > > >     containers because the returned file handles cannot be used.
> > > >
> >
> > Christian,
> >
> > Follow up question:
> > Now that open_by_handle_at(2) is supported from non-root userns,
> > What about this old patch to allow sb/mount watches from non-root userns?
> > https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
> >
> > Is it useful for any of your use cases?
> > Should I push it forward?
>
> Dammit, I answered that message already yesterday but somehow it didn't
> get sent or lost in some other way.
>
> I personally don't have a use-case for it but the systemd folks might
> and it would be best to just rope them in.

Lennart,

I must have asked this question before, but enough time has passed so
I am going to ask it again.

Now that Christian has added support for open_by_handle_at(2) by non-root
userns admin, it is a very low hanging fruit to support fanotify sb/mount
watches inside userns with this simple patch [1], that was last posted in 2011.

My question is whether this is useful, because there are still a few
limitations.
I will start with what is possible with this patch:
1. Watch an entire tmpfs filesystem that was mounted inside userns
2. Watch an entire overlayfs filesystem that was mounted [*] inside userns
3. Watch an entire mount [**] of any [***] filesystem that was
idmapped mounted into userns

Now the the fine prints:
[*] Overlayfs sb/mount CAN be watched, but decoding file handle in
events to path
     only works if overlayfs is mounted with mount option
nfs_export=on, which conflicts
     with mount option metacopy=on, which is often used in containers
(e.g. podman)
[**] Watching a mount is only possible with the legacy set of fanotify events
     (i.e. open,close,access,modify) so this is less useful for
directory tree change tracking
[***] Watching an idmapped mount has the same limitations as watching
an sb/mount
     in the root userns, namely, filesystem needs to have a non zero
fsid (so not FUSE)
     and filesystem needs to have a uniform fsid (so not btrfs
subvolume), although
     with some stretch, I could make watching an idmapped mount of
btrfs subvol work.

No support for watching btrfs subvol and overlayfs with metacopy=on,
reduces the attractiveness for containers, but perhaps there are still use cases
where watching an idmapped mount or userns private tmpfs are useful?

To try out this patch inside your favorite container/userns, you can build
fsnotifywait with a patch to support watching inside userns [2].
It's actually only the one lines O_DIRECTORY patch that is needed for the
basic tmpfs userns mount case.

Jan,

If we do not get any buy-in from potential consumers now, do you think that
we should go through with the patch and advertise the new supported use cases,
so that users may come later on?

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fanotify_userns/
[2] https://github.com/amir73il/inotify-tools/commits/fanotify_userns/
Amir Goldstein Oct. 16, 2024, 12:53 p.m. UTC | #9 
[cc the correct containers list]

On Wed, Oct 16, 2024 at 2:45 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 15, 2024 at 4:01 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Sun, Oct 13, 2024 at 06:34:18PM +0200, Amir Goldstein wrote:
> > > On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > A current limitation of open_by_handle_at() is that it's currently not possible
> > > > > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > > > > in the initial namespace. That's unfortunate because there are scenarios where
> > > > > using open_by_handle_at() from within containers.
> > > > >
> > > > > Two examples:
> > > > >
> > > > > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > > > >     open_by_handle_at().
> > > > > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > > > >     containers because the returned file handles cannot be used.
> > > > >
> > >
> > > Christian,
> > >
> > > Follow up question:
> > > Now that open_by_handle_at(2) is supported from non-root userns,
> > > What about this old patch to allow sb/mount watches from non-root userns?
> > > https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
> > >
> > > Is it useful for any of your use cases?
> > > Should I push it forward?
> >
> > Dammit, I answered that message already yesterday but somehow it didn't
> > get sent or lost in some other way.
> >
> > I personally don't have a use-case for it but the systemd folks might
> > and it would be best to just rope them in.
>
> Lennart,
>
> I must have asked this question before, but enough time has passed so
> I am going to ask it again.
>
> Now that Christian has added support for open_by_handle_at(2) by non-root
> userns admin, it is a very low hanging fruit to support fanotify sb/mount
> watches inside userns with this simple patch [1], that was last posted in 2011.
>
> My question is whether this is useful, because there are still a few
> limitations.
> I will start with what is possible with this patch:
> 1. Watch an entire tmpfs filesystem that was mounted inside userns
> 2. Watch an entire overlayfs filesystem that was mounted [*] inside userns
> 3. Watch an entire mount [**] of any [***] filesystem that was
> idmapped mounted into userns
>
> Now the the fine prints:
> [*] Overlayfs sb/mount CAN be watched, but decoding file handle in
> events to path
>      only works if overlayfs is mounted with mount option
> nfs_export=on, which conflicts
>      with mount option metacopy=on, which is often used in containers
> (e.g. podman)
> [**] Watching a mount is only possible with the legacy set of fanotify events
>      (i.e. open,close,access,modify) so this is less useful for
> directory tree change tracking
> [***] Watching an idmapped mount has the same limitations as watching
> an sb/mount
>      in the root userns, namely, filesystem needs to have a non zero
> fsid (so not FUSE)
>      and filesystem needs to have a uniform fsid (so not btrfs
> subvolume), although
>      with some stretch, I could make watching an idmapped mount of
> btrfs subvol work.
>
> No support for watching btrfs subvol and overlayfs with metacopy=on,
> reduces the attractiveness for containers, but perhaps there are still use cases
> where watching an idmapped mount or userns private tmpfs are useful?
>
> To try out this patch inside your favorite container/userns, you can build
> fsnotifywait with a patch to support watching inside userns [2].
> It's actually only the one lines O_DIRECTORY patch that is needed for the
> basic tmpfs userns mount case.
>
> Jan,
>
> If we do not get any buy-in from potential consumers now, do you think that
> we should go through with the patch and advertise the new supported use cases,
> so that users may come later on?
>
> Thanks,
> Amir.
>
> [1] https://github.com/amir73il/linux/commits/fanotify_userns/
> [2] https://github.com/amir73il/inotify-tools/commits/fanotify_userns/
diff mbox series

Patch

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 07ea3d62b298..b23b052df715 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -427,7 +427,7 @@  EXPORT_SYMBOL_GPL(exportfs_encode_fh);
 
 struct dentry *
 exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
-		       int fileid_type,
+		       int fileid_type, bool directory,
 		       int (*acceptable)(void *, struct dentry *),
 		       void *context)
 {
@@ -445,6 +445,11 @@  exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
 	if (IS_ERR_OR_NULL(result))
 		return result;
 
+	if (directory && !d_is_dir(result)) {
+		err = -ENOTDIR;
+		goto err_result;
+	}
+
 	/*
 	 * If no acceptance criteria was specified by caller, a disconnected
 	 * dentry is also accepatable. Callers may use this mode to query if
@@ -581,7 +586,7 @@  struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 {
 	struct dentry *ret;
 
-	ret = exportfs_decode_fh_raw(mnt, fid, fh_len, fileid_type,
+	ret = exportfs_decode_fh_raw(mnt, fid, fh_len, fileid_type, false,
 				     acceptable, context);
 	if (IS_ERR_OR_NULL(ret)) {
 		if (ret == ERR_PTR(-ENOMEM))
diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8a7f86c2139a..c6ed832ddbb8 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -115,88 +115,174 @@  SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	return err;
 }
 
-static struct vfsmount *get_vfsmount_from_fd(int fd)
+static int get_path_from_fd(int fd, struct path *root)
 {
-	struct vfsmount *mnt;
-
 	if (fd == AT_FDCWD) {
 		struct fs_struct *fs = current->fs;
 		spin_lock(&fs->lock);
-		mnt = mntget(fs->pwd.mnt);
+		*root = fs->pwd;
+		path_get(root);
 		spin_unlock(&fs->lock);
 	} else {
 		struct fd f = fdget(fd);
 		if (!f.file)
-			return ERR_PTR(-EBADF);
-		mnt = mntget(f.file->f_path.mnt);
+			return -EBADF;
+		*root = f.file->f_path;
+		path_get(root);
 		fdput(f);
 	}
-	return mnt;
+
+	return 0;
 }
 
+enum handle_to_path_flags {
+	HANDLE_CHECK_PERMS   = (1 << 0),
+	HANDLE_CHECK_SUBTREE = (1 << 1),
+};
+
+struct handle_to_path_ctx {
+	struct path root;
+	enum handle_to_path_flags flags;
+	bool directory;
+};
+
 static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
 {
-	return 1;
+	struct handle_to_path_ctx *ctx = context;
+	struct user_namespace *user_ns = current_user_ns();
+	struct dentry *d, *root = ctx->root.dentry;
+	struct mnt_idmap *idmap = mnt_idmap(ctx->root.mnt);
+	int retval = 0;
+
+	if (!root)
+		return 1;
+
+	/* Old permission model with global CAP_DAC_READ_SEARCH. */
+	if (!ctx->flags)
+		return 1;
+
+	/*
+	 * It's racy as we're not taking rename_lock but we're able to ignore
+	 * permissions and we just need an approximation whether we were able
+	 * to follow a path to the file.
+	 */
+	d = dget(dentry);
+	while (d != root && !IS_ROOT(d)) {
+		struct dentry *parent = dget_parent(d);
+
+		/*
+		 * We know that we have the ability to override DAC permissions
+		 * as we've verified this earlier via CAP_DAC_READ_SEARCH. But
+		 * we also need to make sure that there aren't any unmapped
+		 * inodes in the path that would prevent us from reaching the
+		 * file.
+		 */
+		if (!privileged_wrt_inode_uidgid(user_ns, idmap,
+						 d_inode(parent))) {
+			dput(d);
+			dput(parent);
+			return retval;
+		}
+
+		dput(d);
+		d = parent;
+	}
+
+	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
+		retval = 1;
+
+	dput(d);
+	return retval;
 }
 
 static int do_handle_to_path(int mountdirfd, struct file_handle *handle,
-			     struct path *path)
+			     struct path *path, struct handle_to_path_ctx *ctx)
 {
-	int retval = 0;
 	int handle_dwords;
+	struct vfsmount *mnt = ctx->root.mnt;
 
-	path->mnt = get_vfsmount_from_fd(mountdirfd);
-	if (IS_ERR(path->mnt)) {
-		retval = PTR_ERR(path->mnt);
-		goto out_err;
-	}
 	/* change the handle size to multiple of sizeof(u32) */
 	handle_dwords = handle->handle_bytes >> 2;
-	path->dentry = exportfs_decode_fh(path->mnt,
+	path->dentry = exportfs_decode_fh_raw(mnt,
 					  (struct fid *)handle->f_handle,
 					  handle_dwords, handle->handle_type,
-					  vfs_dentry_acceptable, NULL);
-	if (IS_ERR(path->dentry)) {
-		retval = PTR_ERR(path->dentry);
-		goto out_mnt;
+					  ctx->directory,
+					  vfs_dentry_acceptable, ctx);
+	if (IS_ERR_OR_NULL(path->dentry)) {
+		if (path->dentry == ERR_PTR(-ENOMEM))
+			return -ENOMEM;
+		return -ESTALE;
 	}
+	path->mnt = mntget(mnt);
 	return 0;
-out_mnt:
-	mntput(path->mnt);
-out_err:
-	return retval;
 }
 
 static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
-		   struct path *path)
+		   struct path *path, unsigned int o_flags)
 {
 	int retval = 0;
 	struct file_handle f_handle;
 	struct file_handle *handle = NULL;
+	struct handle_to_path_ctx ctx = {};
+
+	retval = get_path_from_fd(mountdirfd, &ctx.root);
+	if (retval)
+		goto out_err;
 
-	/*
-	 * With handle we don't look at the execute bit on the
-	 * directory. Ideally we would like CAP_DAC_SEARCH.
-	 * But we don't have that
-	 */
 	if (!capable(CAP_DAC_READ_SEARCH)) {
+		/*
+		 * Allow relaxed permissions of file handles if the caller has
+		 * the ability to mount the filesystem or create a bind-mount
+		 * of the provided @mountdirfd.
+		 *
+		 * In both cases the caller may be able to get an unobstructed
+		 * way to the encoded file handle. If the caller is only able
+		 * to create a bind-mount we need to verify that there are no
+		 * locked mounts on top of it that could prevent us from
+		 * getting to the encoded file.
+		 *
+		 * In principle, locked mounts can prevent the caller from
+		 * mounting the filesystem but that only applies to procfs and
+		 * sysfs neither of which support decoding file handles.
+		 *
+		 * This is currently restricted to O_DIRECTORY to provide a
+		 * deterministic API that avoids a confusing api in the face of
+		 * disconnected non-dir dentries.
+		 */
+
 		retval = -EPERM;
-		goto out_err;
+		if (!(o_flags & O_DIRECTORY))
+			goto out_path;
+
+		if (ns_capable(ctx.root.mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
+			ctx.flags = HANDLE_CHECK_PERMS;
+		else if (ns_capable(real_mount(ctx.root.mnt)->mnt_ns->user_ns, CAP_SYS_ADMIN) &&
+			   !has_locked_children(real_mount(ctx.root.mnt), ctx.root.dentry))
+			ctx.flags = HANDLE_CHECK_PERMS | HANDLE_CHECK_SUBTREE;
+		else
+			goto out_path;
+
+		/* Are we able to override DAC permissions? */
+		if (!ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH))
+			goto out_path;
+
+		ctx.directory = true;
 	}
+
 	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
 		retval = -EFAULT;
-		goto out_err;
+		goto out_path;
 	}
 	if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
 	    (f_handle.handle_bytes == 0)) {
 		retval = -EINVAL;
-		goto out_err;
+		goto out_path;
 	}
 	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
 			 GFP_KERNEL);
 	if (!handle) {
 		retval = -ENOMEM;
-		goto out_err;
+		goto out_path;
 	}
 	/* copy the full handle */
 	*handle = f_handle;
@@ -207,10 +293,14 @@  static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		goto out_handle;
 	}
 
-	retval = do_handle_to_path(mountdirfd, handle, path);
+	retval = do_handle_to_path(mountdirfd, handle, path, &ctx);
+	if (retval)
+		goto out_handle;
 
 out_handle:
 	kfree(handle);
+out_path:
+	path_put(&ctx.root);
 out_err:
 	return retval;
 }
@@ -223,7 +313,7 @@  static long do_handle_open(int mountdirfd, struct file_handle __user *ufh,
 	struct file *file;
 	int fd;
 
-	retval = handle_to_path(mountdirfd, ufh, &path);
+	retval = handle_to_path(mountdirfd, ufh, &path, open_flag);
 	if (retval)
 		return retval;
 
diff --git a/fs/mount.h b/fs/mount.h
index 4a42fc68f4cc..4adce73211ae 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -152,3 +152,4 @@  static inline void move_from_ns(struct mount *mnt, struct list_head *dt_list)
 }
 
 extern void mnt_cursor_del(struct mnt_namespace *ns, struct mount *cursor);
+bool has_locked_children(struct mount *mnt, struct dentry *dentry);
diff --git a/fs/namespace.c b/fs/namespace.c
index 5a51315c6678..4386787210c7 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2078,7 +2078,7 @@  void drop_collected_mounts(struct vfsmount *mnt)
 	namespace_unlock();
 }
 
-static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
+bool has_locked_children(struct mount *mnt, struct dentry *dentry)
 {
 	struct mount *child;
 
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 0b75305fb5f5..3e7f81eb2818 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -247,7 +247,7 @@  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 		dentry = dget(exp->ex_path.dentry);
 	else {
 		dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid,
-						data_left, fileid_type,
+						data_left, fileid_type, false,
 						nfsd_acceptable, exp);
 		if (IS_ERR_OR_NULL(dentry)) {
 			trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index bb37ad5cc954..90c4b0111218 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -305,6 +305,7 @@  static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
 extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
 					     struct fid *fid, int fh_len,
 					     int fileid_type,
+					     bool directory,
 					     int (*acceptable)(void *, struct dentry *),
 					     void *context);
 extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,