* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-05-24 10:19 [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Christian Brauner
@ 2024-05-24 12:35 ` Amir Goldstein
2024-10-13 16:34 ` Amir Goldstein
2024-05-25 10:55 ` [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Jeff Layton
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2024-05-24 12:35 UTC (permalink / raw)
To: Christian Brauner; +Cc: Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-05-24 12:35 ` Amir Goldstein
@ 2024-10-13 16:34 ` Amir Goldstein
2024-10-14 16:06 ` Jan Kara
2024-10-15 14:01 ` Christian Brauner
0 siblings, 2 replies; 14+ messages in thread
From: Amir Goldstein @ 2024-10-13 16:34 UTC (permalink / raw)
To: Christian Brauner, Jan Kara
Cc: Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel
On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > A current limitation of open_by_handle_at() is that it's currently not possible
> > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > in the initial namespace. That's unfortunate because there are scenarios where
> > using open_by_handle_at() from within containers.
> >
> > Two examples:
> >
> > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > open_by_handle_at().
> > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > containers because the returned file handles cannot be used.
> >
Christian,
Follow up question:
Now that open_by_handle_at(2) is supported from non-root userns,
What about this old patch to allow sb/mount watches from non-root userns?
https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
Is it useful for any of your use cases?
Should I push it forward?
I have rebased and sanity tested it:
https://github.com/amir73il/linux/commits/fanotify_userns/
But I have not written any userns fanotify test yet.
If you are interested, please let me know.
If you can test the patch for your use case even better.
BTW, the prep patch that I refactored per Jan's review request
is almost identical to your patch from 2019:
https://lore.kernel.org/linux-fsdevel/20190522163150.16849-1-christian@brauner.io/
But with additional BUILD_BUG_ON and a commit message to explain it.
> > Here's a proposal for relaxing the permission check for open_by_handle_at().
> >
> > (1) Opening file handles when the caller has privileges over the filesystem
> > (1.1) The caller has an unobstructed view of the filesystem.
> > (1.2) The caller has permissions to follow a path to the file handle.
> >
> > This doesn't address the problem of opening a file handle when only a portion
> > of a filesystem is exposed as is common in containers by e.g., bind-mounting a
> > subtree. The proposal to solve this use-case is:
> >
> > (2) Opening file handles when the caller has privileges over a subtree
> > (2.1) The caller is able to reach the file from the provided mount fd.
> > (2.2) The caller has permissions to construct an unobstructed path to the
> > file handle.
> > (2.3) The caller has permissions to follow a path to the file handle.
> >
> > The relaxed permission checks are currently restricted to directory file
> > handles which are what both cgroupfs and fanotify need. Handling disconnected
> > non-directory file handles would lead to a potentially non-deterministic api.
> > If a disconnected non-directory file handle is provided we may fail to decode
> > a valid path that we could use for permission checking. That in itself isn't a
> > problem as we would just return EACCES in that case. However, confusion may
> > arise if a non-disconnected dentry ends up in the cache later and those opening
> > the file handle would suddenly succeed.
> >
> > * It's potentially possible to use timing information (side-channel) to infer
> > whether a given inode exists. I don't think that's particularly
> > problematic. Thanks to Jann for bringing this to my attention.
> >
> > * An unrelated note (IOW, these are thoughts that apply to
> > open_by_handle_at() generically and are unrelated to the changes here):
> > Jann pointed out that we should verify whether deleted files could
> > potentially be reopened through open_by_handle_at(). I don't think that's
> > possible though.
>
> AFAICT, the only thing that currently makes this impossible in this patch
> is the O_DIRECTORY limitation.
> Because there could be non-directory non-hashed aliases of deleted
> files in dcache.
>
This limitation, that you added to my request, has implications on
fanotify (TLDR):
file handles reported in fanotify events extra info of type
FAN_EVENT_INFO_TYPE_DFID{,_NAME} due to fanotify_inide()
flag FAN_REPORT_DIR_FID are eligible for open_by_handle_at(2)
inside non-root userns and FAN_REPORT_DFID_NAME is indeed
the most expected use case - namely
directory file handle can always be used to find the parent's path
and the name of the child entry is provided in the event.
HOWEVER,
with fanotify_init() flags FAN_REPORT_{TARGET_,}FID
the child file handle is reported additionally, using extra info type
FAN_EVENT_INFO_TYPE_FID and this file handle may not be
eligible for open_by_handle_at(2) inside non-root userns,
because it could be resolved to a disconnected dentry, for which
permission cannot be checked.
I don't think this is a problem - the child fid is supposed to
be used as an extra check to verify with name_to_handle_at()
that the parent dir and child name refer to the same object as
the one reported with the child fid.
But it can be a bit confusing to users that some file handles
(FAN_EVENT_INFO_TYPE_DFID) can be decoded and some
(FAN_EVENT_INFO_TYPE_FID) cannot, so this hairy detail
will need to be documented.
In retrospect, for unpriv fanotify, we should not have allowed
the basic mode FAN_REPORT_FID, which does not make a
distinction between directory and non-dir file handles.
Jan, do you think we should deny the FAN_REPORT_FID mode
with non-root userns sb/mount watches to reduce some unneeded
rows in the test matrix?
Any other thoughts regarding non-root userns sb/mount watches?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-10-13 16:34 ` Amir Goldstein
@ 2024-10-14 16:06 ` Jan Kara
2024-10-15 14:01 ` Christian Brauner
1 sibling, 0 replies; 14+ messages in thread
From: Jan Kara @ 2024-10-14 16:06 UTC (permalink / raw)
To: Amir Goldstein
Cc: Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Aleksa Sarai, linux-fsdevel
On Sun 13-10-24 18:34:18, Amir Goldstein wrote:
> On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > AFAICT, the only thing that currently makes this impossible in this patch
> > is the O_DIRECTORY limitation.
> > Because there could be non-directory non-hashed aliases of deleted
> > files in dcache.
> >
>
> This limitation, that you added to my request, has implications on
> fanotify (TLDR):
>
> file handles reported in fanotify events extra info of type
> FAN_EVENT_INFO_TYPE_DFID{,_NAME} due to fanotify_inide()
> flag FAN_REPORT_DIR_FID are eligible for open_by_handle_at(2)
> inside non-root userns and FAN_REPORT_DFID_NAME is indeed
> the most expected use case - namely
> directory file handle can always be used to find the parent's path
> and the name of the child entry is provided in the event.
>
> HOWEVER,
> with fanotify_init() flags FAN_REPORT_{TARGET_,}FID
> the child file handle is reported additionally, using extra info type
> FAN_EVENT_INFO_TYPE_FID and this file handle may not be
> eligible for open_by_handle_at(2) inside non-root userns,
> because it could be resolved to a disconnected dentry, for which
> permission cannot be checked.
>
> I don't think this is a problem - the child fid is supposed to
> be used as an extra check to verify with name_to_handle_at()
> that the parent dir and child name refer to the same object as
> the one reported with the child fid.
> But it can be a bit confusing to users that some file handles
> (FAN_EVENT_INFO_TYPE_DFID) can be decoded and some
> (FAN_EVENT_INFO_TYPE_FID) cannot, so this hairy detail
> will need to be documented.
Well, I think the explanation that open_by_handle_at() works only for
directories inside containers is clear enough explanation. It has nothing
to do with whether the FID comes from FAN_EVENT_INFO_TYPE_DFID or from
FAN_EVENT_INFO_TYPE_FID.
> In retrospect, for unpriv fanotify, we should not have allowed
> the basic mode FAN_REPORT_FID, which does not make a
> distinction between directory and non-dir file handles.
I don't think we could have forseen these changes. Also I don't think the
interface is wrong per se. Just the handle cannot be used with
open_by_handle_at() if it points to a regular file so it may be annoying to
use but we didn't expect the handle will ever be usable with
open_by_handle_at() for unpriviledged users so making it usable for
directories can be seen as an improvement :). Realistically I think
unpriviledged users will use FAN_EVENT_INFO_TYPE_DFID_NAME to avoid these
headaches.
> Jan, do you think we should deny the FAN_REPORT_FID mode
> with non-root userns sb/mount watches to reduce some unneeded
> rows in the test matrix?
I think this would be a bit arbitrary restriction. I don't see how
supporting FAN_REPORT_FID would be making our life any harder.
> Any other thoughts regarding non-root userns sb/mount watches?
No, not really.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-10-13 16:34 ` Amir Goldstein
2024-10-14 16:06 ` Jan Kara
@ 2024-10-15 14:01 ` Christian Brauner
2024-10-16 12:45 ` fanotify sb/mount watch inside userns (Was: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks) Amir Goldstein
1 sibling, 1 reply; 14+ messages in thread
From: Christian Brauner @ 2024-10-15 14:01 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel
On Sun, Oct 13, 2024 at 06:34:18PM +0200, Amir Goldstein wrote:
> On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > A current limitation of open_by_handle_at() is that it's currently not possible
> > > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > > in the initial namespace. That's unfortunate because there are scenarios where
> > > using open_by_handle_at() from within containers.
> > >
> > > Two examples:
> > >
> > > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > > open_by_handle_at().
> > > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > > containers because the returned file handles cannot be used.
> > >
>
> Christian,
>
> Follow up question:
> Now that open_by_handle_at(2) is supported from non-root userns,
> What about this old patch to allow sb/mount watches from non-root userns?
> https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
>
> Is it useful for any of your use cases?
> Should I push it forward?
Dammit, I answered that message already yesterday but somehow it didn't
get sent or lost in some other way.
I personally don't have a use-case for it but the systemd folks might
and it would be best to just rope them in.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: fanotify sb/mount watch inside userns (Was: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks)
2024-10-15 14:01 ` Christian Brauner
@ 2024-10-16 12:45 ` Amir Goldstein
2024-10-16 12:53 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2024-10-16 12:45 UTC (permalink / raw)
To: Lennart Poettering, Jan Kara
Cc: Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel,
Linux Containers, Christian Brauner
On Tue, Oct 15, 2024 at 4:01 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sun, Oct 13, 2024 at 06:34:18PM +0200, Amir Goldstein wrote:
> > On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > A current limitation of open_by_handle_at() is that it's currently not possible
> > > > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > > > in the initial namespace. That's unfortunate because there are scenarios where
> > > > using open_by_handle_at() from within containers.
> > > >
> > > > Two examples:
> > > >
> > > > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > > > open_by_handle_at().
> > > > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > > > containers because the returned file handles cannot be used.
> > > >
> >
> > Christian,
> >
> > Follow up question:
> > Now that open_by_handle_at(2) is supported from non-root userns,
> > What about this old patch to allow sb/mount watches from non-root userns?
> > https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
> >
> > Is it useful for any of your use cases?
> > Should I push it forward?
>
> Dammit, I answered that message already yesterday but somehow it didn't
> get sent or lost in some other way.
>
> I personally don't have a use-case for it but the systemd folks might
> and it would be best to just rope them in.
Lennart,
I must have asked this question before, but enough time has passed so
I am going to ask it again.
Now that Christian has added support for open_by_handle_at(2) by non-root
userns admin, it is a very low hanging fruit to support fanotify sb/mount
watches inside userns with this simple patch [1], that was last posted in 2011.
My question is whether this is useful, because there are still a few
limitations.
I will start with what is possible with this patch:
1. Watch an entire tmpfs filesystem that was mounted inside userns
2. Watch an entire overlayfs filesystem that was mounted [*] inside userns
3. Watch an entire mount [**] of any [***] filesystem that was
idmapped mounted into userns
Now the the fine prints:
[*] Overlayfs sb/mount CAN be watched, but decoding file handle in
events to path
only works if overlayfs is mounted with mount option
nfs_export=on, which conflicts
with mount option metacopy=on, which is often used in containers
(e.g. podman)
[**] Watching a mount is only possible with the legacy set of fanotify events
(i.e. open,close,access,modify) so this is less useful for
directory tree change tracking
[***] Watching an idmapped mount has the same limitations as watching
an sb/mount
in the root userns, namely, filesystem needs to have a non zero
fsid (so not FUSE)
and filesystem needs to have a uniform fsid (so not btrfs
subvolume), although
with some stretch, I could make watching an idmapped mount of
btrfs subvol work.
No support for watching btrfs subvol and overlayfs with metacopy=on,
reduces the attractiveness for containers, but perhaps there are still use cases
where watching an idmapped mount or userns private tmpfs are useful?
To try out this patch inside your favorite container/userns, you can build
fsnotifywait with a patch to support watching inside userns [2].
It's actually only the one lines O_DIRECTORY patch that is needed for the
basic tmpfs userns mount case.
Jan,
If we do not get any buy-in from potential consumers now, do you think that
we should go through with the patch and advertise the new supported use cases,
so that users may come later on?
Thanks,
Amir.
[1] https://github.com/amir73il/linux/commits/fanotify_userns/
[2] https://github.com/amir73il/inotify-tools/commits/fanotify_userns/
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: fanotify sb/mount watch inside userns (Was: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks)
2024-10-16 12:45 ` fanotify sb/mount watch inside userns (Was: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks) Amir Goldstein
@ 2024-10-16 12:53 ` Amir Goldstein
2025-04-18 11:32 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2024-10-16 12:53 UTC (permalink / raw)
To: Lennart Poettering, Jan Kara
Cc: Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel,
Christian Brauner, containers
[cc the correct containers list]
On Wed, Oct 16, 2024 at 2:45 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 15, 2024 at 4:01 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Sun, Oct 13, 2024 at 06:34:18PM +0200, Amir Goldstein wrote:
> > > On Fri, May 24, 2024 at 2:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Fri, May 24, 2024 at 1:19 PM Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > A current limitation of open_by_handle_at() is that it's currently not possible
> > > > > to use it from within containers at all because we require CAP_DAC_READ_SEARCH
> > > > > in the initial namespace. That's unfortunate because there are scenarios where
> > > > > using open_by_handle_at() from within containers.
> > > > >
> > > > > Two examples:
> > > > >
> > > > > (1) cgroupfs allows to encode cgroups to file handles and reopen them with
> > > > > open_by_handle_at().
> > > > > (2) Fanotify allows placing filesystem watches they currently aren't usable in
> > > > > containers because the returned file handles cannot be used.
> > > > >
> > >
> > > Christian,
> > >
> > > Follow up question:
> > > Now that open_by_handle_at(2) is supported from non-root userns,
> > > What about this old patch to allow sb/mount watches from non-root userns?
> > > https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
> > >
> > > Is it useful for any of your use cases?
> > > Should I push it forward?
> >
> > Dammit, I answered that message already yesterday but somehow it didn't
> > get sent or lost in some other way.
> >
> > I personally don't have a use-case for it but the systemd folks might
> > and it would be best to just rope them in.
>
> Lennart,
>
> I must have asked this question before, but enough time has passed so
> I am going to ask it again.
>
> Now that Christian has added support for open_by_handle_at(2) by non-root
> userns admin, it is a very low hanging fruit to support fanotify sb/mount
> watches inside userns with this simple patch [1], that was last posted in 2011.
>
> My question is whether this is useful, because there are still a few
> limitations.
> I will start with what is possible with this patch:
> 1. Watch an entire tmpfs filesystem that was mounted inside userns
> 2. Watch an entire overlayfs filesystem that was mounted [*] inside userns
> 3. Watch an entire mount [**] of any [***] filesystem that was
> idmapped mounted into userns
>
> Now the the fine prints:
> [*] Overlayfs sb/mount CAN be watched, but decoding file handle in
> events to path
> only works if overlayfs is mounted with mount option
> nfs_export=on, which conflicts
> with mount option metacopy=on, which is often used in containers
> (e.g. podman)
> [**] Watching a mount is only possible with the legacy set of fanotify events
> (i.e. open,close,access,modify) so this is less useful for
> directory tree change tracking
> [***] Watching an idmapped mount has the same limitations as watching
> an sb/mount
> in the root userns, namely, filesystem needs to have a non zero
> fsid (so not FUSE)
> and filesystem needs to have a uniform fsid (so not btrfs
> subvolume), although
> with some stretch, I could make watching an idmapped mount of
> btrfs subvol work.
>
> No support for watching btrfs subvol and overlayfs with metacopy=on,
> reduces the attractiveness for containers, but perhaps there are still use cases
> where watching an idmapped mount or userns private tmpfs are useful?
>
> To try out this patch inside your favorite container/userns, you can build
> fsnotifywait with a patch to support watching inside userns [2].
> It's actually only the one lines O_DIRECTORY patch that is needed for the
> basic tmpfs userns mount case.
>
> Jan,
>
> If we do not get any buy-in from potential consumers now, do you think that
> we should go through with the patch and advertise the new supported use cases,
> so that users may come later on?
>
> Thanks,
> Amir.
>
> [1] https://github.com/amir73il/linux/commits/fanotify_userns/
> [2] https://github.com/amir73il/inotify-tools/commits/fanotify_userns/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: fanotify sb/mount watch inside userns (Was: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks)
2024-10-16 12:53 ` Amir Goldstein
@ 2025-04-18 11:32 ` Amir Goldstein
2025-04-24 11:28 ` Jan Kara
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2025-04-18 11:32 UTC (permalink / raw)
To: Lennart Poettering, Jan Kara, Miklos Szeredi, Christian Brauner
Cc: Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel, containers
On Wed, Oct 16, 2024 at 2:53 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
...
> > > > Christian,
> > > >
> > > > Follow up question:
> > > > Now that open_by_handle_at(2) is supported from non-root userns,
> > > > What about this old patch [1] to allow sb/mount watches from non-root userns?
> > > >
> > > >
...
> >
> > My question is whether this is useful, because there are still a few
> > limitations.
> > I will start with what is possible with this patch:
> > 1. Watch an entire tmpfs filesystem that was mounted inside userns
> > 2. Watch an entire overlayfs filesystem that was mounted [*] inside userns
> > 3. Watch an entire mount [**] of any [***] filesystem that was
> > idmapped mounted into userns
> >
> > Now the the fine prints:
> > [*] Overlayfs sb/mount CAN be watched, but decoding file handle in
> > events to path
> > only works if overlayfs is mounted with mount option
> > nfs_export=on, which conflicts
> > with mount option metacopy=on, which is often used in containers
> > (e.g. podman)
> > [**] Watching a mount is only possible with the legacy set of fanotify events
> > (i.e. open,close,access,modify) so this is less useful for
> > directory tree change tracking
> > [***] Watching an idmapped mount has the same limitations as watching
> > an sb/mount
> > in the root userns, namely, filesystem needs to have a non zero
> > fsid (so not FUSE)
> > and filesystem needs to have a uniform fsid (so not btrfs
> > subvolume), although
> > with some stretch, I could make watching an idmapped mount of
> > btrfs subvol work.
> >
> > No support for watching btrfs subvol and overlayfs with metacopy=on,
> > reduces the attractiveness for containers, but perhaps there are still use cases
> > where watching an idmapped mount or userns private tmpfs are useful?
> >
> > To try out this patch inside your favorite container/userns, you can build
> > fsnotifywait with a patch to support watching inside userns [2].
> > It's actually only the one lines O_DIRECTORY patch that is needed for the
> > basic tmpfs userns mount case.
> >
> > Jan,
> >
> > If we do not get any buy-in from potential consumers now, do you think that
> > we should go through with the patch and advertise the new supported use cases,
> > so that users may come later on?
> >
Hi guys,
The fine print section in the above "is it useful" question is quite complex.
Maybe that explains why I got no response ;)
I can now ask another flavor of the question:
Is it useful to allow FAN_MARK_MNTNS watches from non-root userns?
Just to make sure that I get it right, the unique mntid that is reported with
mount attach/detach events is not namespaced right?
Anyway, this functionality has become a priority for me, so I will
work on adapting the old patch [3] to mntns marks and post v2.
Thanks,
Amir.
[1] https://github.com/amir73il/linux/commits/fanotify_userns/
[2] https://github.com/amir73il/inotify-tools/commits/fanotify_userns/
[3] https://lore.kernel.org/linux-fsdevel/20230416060722.1912831-1-amir73il@gmail.com/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: fanotify sb/mount watch inside userns (Was: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks)
2025-04-18 11:32 ` Amir Goldstein
@ 2025-04-24 11:28 ` Jan Kara
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2025-04-24 11:28 UTC (permalink / raw)
To: Amir Goldstein
Cc: Lennart Poettering, Jan Kara, Miklos Szeredi, Christian Brauner,
Jeff Layton, Chuck Lever, Aleksa Sarai, linux-fsdevel, containers
On Fri 18-04-25 13:32:48, Amir Goldstein wrote:
> On Wed, Oct 16, 2024 at 2:53 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> ...
> > > > > Christian,
> > > > >
> > > > > Follow up question:
> > > > > Now that open_by_handle_at(2) is supported from non-root userns,
> > > > > What about this old patch [1] to allow sb/mount watches from non-root userns?
> > > > >
> > > > >
> ...
> > >
> > > My question is whether this is useful, because there are still a few
> > > limitations.
> > > I will start with what is possible with this patch:
> > > 1. Watch an entire tmpfs filesystem that was mounted inside userns
> > > 2. Watch an entire overlayfs filesystem that was mounted [*] inside userns
> > > 3. Watch an entire mount [**] of any [***] filesystem that was
> > > idmapped mounted into userns
> > >
> > > Now the the fine prints:
> > > [*] Overlayfs sb/mount CAN be watched, but decoding file handle in
> > > events to path
> > > only works if overlayfs is mounted with mount option
> > > nfs_export=on, which conflicts
> > > with mount option metacopy=on, which is often used in containers
> > > (e.g. podman)
> > > [**] Watching a mount is only possible with the legacy set of fanotify events
> > > (i.e. open,close,access,modify) so this is less useful for
> > > directory tree change tracking
> > > [***] Watching an idmapped mount has the same limitations as watching
> > > an sb/mount
> > > in the root userns, namely, filesystem needs to have a non zero
> > > fsid (so not FUSE)
> > > and filesystem needs to have a uniform fsid (so not btrfs
> > > subvolume), although
> > > with some stretch, I could make watching an idmapped mount of
> > > btrfs subvol work.
> > >
> > > No support for watching btrfs subvol and overlayfs with metacopy=on,
> > > reduces the attractiveness for containers, but perhaps there are still use cases
> > > where watching an idmapped mount or userns private tmpfs are useful?
> > >
> > > To try out this patch inside your favorite container/userns, you can build
> > > fsnotifywait with a patch to support watching inside userns [2].
> > > It's actually only the one lines O_DIRECTORY patch that is needed for the
> > > basic tmpfs userns mount case.
> > >
> > > Jan,
> > >
> > > If we do not get any buy-in from potential consumers now, do you think that
> > > we should go through with the patch and advertise the new supported use cases,
> > > so that users may come later on?
> > >
>
> Hi guys,
>
> The fine print section in the above "is it useful" question is quite complex.
> Maybe that explains why I got no response ;)
>
> I can now ask another flavor of the question:
>
> Is it useful to allow FAN_MARK_MNTNS watches from non-root userns?
Yes, I think these are useful but one has to be careful about not leaking
information from other namespaces.
> Just to make sure that I get it right, the unique mntid that is reported with
> mount attach/detach events is not namespaced right?
Correct. It is not namespaced.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-05-24 10:19 [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Christian Brauner
2024-05-24 12:35 ` Amir Goldstein
@ 2024-05-25 10:55 ` Jeff Layton
2024-05-27 2:49 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Jeff Layton @ 2024-05-25 10:55 UTC (permalink / raw)
To: Christian Brauner, Amir Goldstein, Chuck Lever, Aleksa Sarai
Cc: linux-fsdevel
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>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-05-24 10:19 [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Christian Brauner
2024-05-24 12:35 ` Amir Goldstein
2024-05-25 10:55 ` [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Jeff Layton
@ 2024-05-27 2:49 ` kernel test robot
2024-05-27 7:28 ` Dan Carpenter
2024-05-27 11:31 ` Christoph Hellwig
4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2024-05-27 2:49 UTC (permalink / raw)
To: Christian Brauner
Cc: oe-lkp, lkp, linux-fsdevel, linux-nfs, Amir Goldstein,
Jeff Layton, Chuck Lever, Aleksa Sarai, Christian Brauner,
oliver.sang
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-05-24 10:19 [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Christian Brauner
` (2 preceding siblings ...)
2024-05-27 2:49 ` kernel test robot
@ 2024-05-27 7:28 ` Dan Carpenter
2024-05-27 11:31 ` Christoph Hellwig
4 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2024-05-27 7:28 UTC (permalink / raw)
To: oe-kbuild, Christian Brauner; +Cc: lkp, oe-kbuild-all
Hi Christian,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Brauner/fhandle-relax-open_by_handle_at-permission-checks/20240524-182059
base: 8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6
patch link: https://lore.kernel.org/r/20240524-vfs-open_by_handle_at-v1-1-3d4b7d22736b%40kernel.org
patch subject: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
config: i386-randconfig-141-20240525 (https://download.01.org/0day-ci/archive/20240526/202405260707.w8Nh9Nv6-lkp@intel.com/config)
compiler: gcc-12 (Ubuntu 12.3.0-9ubuntu2) 12.3.0
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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202405260707.w8Nh9Nv6-lkp@intel.com/
smatch warnings:
fs/fhandle.c:296 handle_to_path() warn: fd re-used after fget(): 'mountdirfd'
vim +/mountdirfd +296 fs/fhandle.c
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 220 static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
9ca8b65e411ba7 Christian Brauner 2024-05-24 221 struct path *path, unsigned int o_flags)
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 222 {
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 223 int retval = 0;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 224 struct file_handle f_handle;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 225 struct file_handle *handle = NULL;
9ca8b65e411ba7 Christian Brauner 2024-05-24 226 struct handle_to_path_ctx ctx = {};
9ca8b65e411ba7 Christian Brauner 2024-05-24 227
9ca8b65e411ba7 Christian Brauner 2024-05-24 228 retval = get_path_from_fd(mountdirfd, &ctx.root);
^^^^^^^^^^
fd used here.
9ca8b65e411ba7 Christian Brauner 2024-05-24 229 if (retval)
9ca8b65e411ba7 Christian Brauner 2024-05-24 230 goto out_err;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 231
9ca8b65e411ba7 Christian Brauner 2024-05-24 232 if (!capable(CAP_DAC_READ_SEARCH)) {
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 233 /*
9ca8b65e411ba7 Christian Brauner 2024-05-24 234 * Allow relaxed permissions of file handles if the caller has
9ca8b65e411ba7 Christian Brauner 2024-05-24 235 * the ability to mount the filesystem or create a bind-mount
9ca8b65e411ba7 Christian Brauner 2024-05-24 236 * of the provided @mountdirfd.
9ca8b65e411ba7 Christian Brauner 2024-05-24 237 *
9ca8b65e411ba7 Christian Brauner 2024-05-24 238 * In both cases the caller may be able to get an unobstructed
9ca8b65e411ba7 Christian Brauner 2024-05-24 239 * way to the encoded file handle. If the caller is only able
9ca8b65e411ba7 Christian Brauner 2024-05-24 240 * to create a bind-mount we need to verify that there are no
9ca8b65e411ba7 Christian Brauner 2024-05-24 241 * locked mounts on top of it that could prevent us from
9ca8b65e411ba7 Christian Brauner 2024-05-24 242 * getting to the encoded file.
9ca8b65e411ba7 Christian Brauner 2024-05-24 243 *
9ca8b65e411ba7 Christian Brauner 2024-05-24 244 * In principle, locked mounts can prevent the caller from
9ca8b65e411ba7 Christian Brauner 2024-05-24 245 * mounting the filesystem but that only applies to procfs and
9ca8b65e411ba7 Christian Brauner 2024-05-24 246 * sysfs neither of which support decoding file handles.
9ca8b65e411ba7 Christian Brauner 2024-05-24 247 *
9ca8b65e411ba7 Christian Brauner 2024-05-24 248 * This is currently restricted to O_DIRECTORY to provide a
9ca8b65e411ba7 Christian Brauner 2024-05-24 249 * deterministic API that avoids a confusing api in the face of
9ca8b65e411ba7 Christian Brauner 2024-05-24 250 * disconnected non-dir dentries.
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 251 */
9ca8b65e411ba7 Christian Brauner 2024-05-24 252
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 253 retval = -EPERM;
9ca8b65e411ba7 Christian Brauner 2024-05-24 254 if (!(o_flags & O_DIRECTORY))
9ca8b65e411ba7 Christian Brauner 2024-05-24 255 goto out_path;
9ca8b65e411ba7 Christian Brauner 2024-05-24 256
9ca8b65e411ba7 Christian Brauner 2024-05-24 257 if (ns_capable(ctx.root.mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
9ca8b65e411ba7 Christian Brauner 2024-05-24 258 ctx.flags = HANDLE_CHECK_PERMS;
9ca8b65e411ba7 Christian Brauner 2024-05-24 259 else if (ns_capable(real_mount(ctx.root.mnt)->mnt_ns->user_ns, CAP_SYS_ADMIN) &&
9ca8b65e411ba7 Christian Brauner 2024-05-24 260 !has_locked_children(real_mount(ctx.root.mnt), ctx.root.dentry))
9ca8b65e411ba7 Christian Brauner 2024-05-24 261 ctx.flags = HANDLE_CHECK_PERMS | HANDLE_CHECK_SUBTREE;
9ca8b65e411ba7 Christian Brauner 2024-05-24 262 else
9ca8b65e411ba7 Christian Brauner 2024-05-24 263 goto out_path;
9ca8b65e411ba7 Christian Brauner 2024-05-24 264
9ca8b65e411ba7 Christian Brauner 2024-05-24 265 /* Are we able to override DAC permissions? */
9ca8b65e411ba7 Christian Brauner 2024-05-24 266 if (!ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH))
9ca8b65e411ba7 Christian Brauner 2024-05-24 267 goto out_path;
9ca8b65e411ba7 Christian Brauner 2024-05-24 268
9ca8b65e411ba7 Christian Brauner 2024-05-24 269 ctx.directory = true;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 270 }
9ca8b65e411ba7 Christian Brauner 2024-05-24 271
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 272 if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 273 retval = -EFAULT;
9ca8b65e411ba7 Christian Brauner 2024-05-24 274 goto out_path;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 275 }
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 276 if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 277 (f_handle.handle_bytes == 0)) {
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 278 retval = -EINVAL;
9ca8b65e411ba7 Christian Brauner 2024-05-24 279 goto out_path;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 280 }
68d6f4f3fbd9b1 Gustavo A. R. Silva 2024-03-25 281 handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 282 GFP_KERNEL);
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 283 if (!handle) {
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 284 retval = -ENOMEM;
9ca8b65e411ba7 Christian Brauner 2024-05-24 285 goto out_path;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 286 }
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 287 /* copy the full handle */
161f873b89136e Sasha Levin 2015-01-28 288 *handle = f_handle;
161f873b89136e Sasha Levin 2015-01-28 289 if (copy_from_user(&handle->f_handle,
161f873b89136e Sasha Levin 2015-01-28 290 &ufh->f_handle,
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 291 f_handle.handle_bytes)) {
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 292 retval = -EFAULT;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 293 goto out_handle;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 294 }
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 295
9ca8b65e411ba7 Christian Brauner 2024-05-24 @296 retval = do_handle_to_path(mountdirfd, handle, path, &ctx);
^^^^^^^^^^
re-used here. The fd could point to different files for each fget().
9ca8b65e411ba7 Christian Brauner 2024-05-24 297 if (retval)
9ca8b65e411ba7 Christian Brauner 2024-05-24 298 goto out_handle;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 299
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 300 out_handle:
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 301 kfree(handle);
9ca8b65e411ba7 Christian Brauner 2024-05-24 302 out_path:
9ca8b65e411ba7 Christian Brauner 2024-05-24 303 path_put(&ctx.root);
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 304 out_err:
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 305 return retval;
becfd1f3754479 Aneesh Kumar K.V 2011-01-29 306 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks
2024-05-24 10:19 [PATCH RFC] : fhandle: relax open_by_handle_at() permission checks Christian Brauner
` (3 preceding siblings ...)
2024-05-27 7:28 ` Dan Carpenter
@ 2024-05-27 11:31 ` Christoph Hellwig
4 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2024-05-27 11:31 UTC (permalink / raw)
To: Christian Brauner
Cc: Amir Goldstein, Jeff Layton, Chuck Lever, Aleksa Sarai,
linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread