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.
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,