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