| Message ID | 20241008152118.453724-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | API for exporting connectable file handles to userspace | expand |
On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote: > We would like to use the high 16bit of the handle_type field to encode > file handle traits, such as "connectable". > > In preparation for this change, make sure that filesystems do not return > a handle_type value with upper bits set and that the open_by_handle_at(2) > syscall rejects these handle types. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/exportfs/expfs.c | 14 ++++++++++++-- > fs/fhandle.c | 6 ++++++ > include/linux/exportfs.h | 14 ++++++++++++++ > 3 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c > index 4f2dd4ab4486..c8eb660fdde4 100644 > --- a/fs/exportfs/expfs.c > +++ b/fs/exportfs/expfs.c > @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, > int *max_len, struct inode *parent, int flags) > { > const struct export_operations *nop = inode->i_sb->s_export_op; > + enum fid_type type; > > if (!exportfs_can_encode_fh(nop, flags)) > return -EOPNOTSUPP; > > if (!nop && (flags & EXPORT_FH_FID)) > - return exportfs_encode_ino64_fid(inode, fid, max_len); > + type = exportfs_encode_ino64_fid(inode, fid, max_len); > + else > + type = nop->encode_fh(inode, fid->raw, max_len, parent); > + > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(type))) > + return -EINVAL; > + The stack trace won't be very useful here. Rather than a WARN, it might be better to dump out some info about the fstype (and maybe other info?) that returned the bogus type value here. I'm pretty sure most in-kernel fs's don't do this, but who knows what 3rd party fs's might do. > + return type; > > - return nop->encode_fh(inode, fid->raw, max_len, parent); > } > EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh); > > @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len, > char nbuf[NAME_MAX+1]; > int err; > > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type))) > + return -EINVAL; > + This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which means that this fh comes from userland or from an NFS client. I don't think we want to WARN because someone crafted a bogus fh and passed it to us. > /* > * Try to get any dentry for the given file handle from the filesystem. > */ > diff --git a/fs/fhandle.c b/fs/fhandle.c > index 82df28d45cd7..c5792cf3c6e9 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -307,6 +307,10 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, > retval = -EINVAL; > goto out_path; > } > + if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) { > + retval = -EINVAL; > + goto out_path; > + } > handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes), > GFP_KERNEL); > if (!handle) { > @@ -322,6 +326,8 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, > goto out_handle; > } > > + /* Filesystem code should not be exposed to user flags */ > + handle->handle_type &= ~FILEID_USER_FLAGS_MASK; > retval = do_handle_to_path(handle, path, &ctx); > > out_handle: > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index 893a1d21dc1c..76a3050b3593 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -160,6 +160,20 @@ struct fid { > #define EXPORT_FH_FID 0x2 /* File handle may be non-decodeable */ > #define EXPORT_FH_DIR_ONLY 0x4 /* Only decode file handle for a directory */ > > +/* > + * Filesystems use only lower 8 bits of file_handle type for fid_type. > + * name_to_handle_at() uses upper 16 bits of type as user flags to be > + * interpreted by open_by_handle_at(). > + */ > +#define FILEID_USER_FLAGS_MASK 0xffff0000 > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > + > +/* Flags supported in encoded handle_type that is exported to user */ > +#define FILEID_VALID_USER_FLAGS (0) > + > +#define FILEID_USER_TYPE_IS_VALID(type) \ > + (!(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)) > + > /** > * struct export_operations - for nfsd to communicate with file systems > * @encode_fh: encode a file handle fragment from a dentry The rest looks reasonable.
On Tue, Oct 8, 2024 at 8:19 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote: > > We would like to use the high 16bit of the handle_type field to encode > > file handle traits, such as "connectable". > > > > In preparation for this change, make sure that filesystems do not return > > a handle_type value with upper bits set and that the open_by_handle_at(2) > > syscall rejects these handle types. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/exportfs/expfs.c | 14 ++++++++++++-- > > fs/fhandle.c | 6 ++++++ > > include/linux/exportfs.h | 14 ++++++++++++++ > > 3 files changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c > > index 4f2dd4ab4486..c8eb660fdde4 100644 > > --- a/fs/exportfs/expfs.c > > +++ b/fs/exportfs/expfs.c > > @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, > > int *max_len, struct inode *parent, int flags) > > { > > const struct export_operations *nop = inode->i_sb->s_export_op; > > + enum fid_type type; > > > > if (!exportfs_can_encode_fh(nop, flags)) > > return -EOPNOTSUPP; > > > > if (!nop && (flags & EXPORT_FH_FID)) > > - return exportfs_encode_ino64_fid(inode, fid, max_len); > > + type = exportfs_encode_ino64_fid(inode, fid, max_len); > > + else > > + type = nop->encode_fh(inode, fid->raw, max_len, parent); > > + > > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(type))) > > + return -EINVAL; > > + > > The stack trace won't be very useful here. Rather than a WARN, it might > be better to dump out some info about the fstype (and maybe other > info?) that returned the bogus type value here. I'm pretty sure most > in-kernel fs's don't do this, but who knows what 3rd party fs's might > do. > Right. I changed to: if (FILEID_USER_FLAGS(type)) { pr_warn_once("%s: unexpected fh type value 0x%x from fstype %s.\n", __func__, type, inode->i_sb->s_type->name); return -EINVAL; } > > + return type; > > > > - return nop->encode_fh(inode, fid->raw, max_len, parent); > > } > > EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh); > > > > @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len, > > char nbuf[NAME_MAX+1]; > > int err; > > > > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type))) > > + return -EINVAL; > > + > > > This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which > means that this fh comes from userland or from an NFS client. I don't > think we want to WARN because someone crafted a bogus fh and passed it > to us. > Good point, I will remove the WARN and also fix the bug :-/ if (FILEID_USER_FLAGS(fileid_type)) return ERR_PTR(-EINVAL); Pushed this and othe minor fixes to https://github.com/amir73il/linux/commits/connectable-fh/ until we sort out the rest of your comments and maybe get more feedback. Thanks for the review! Amir.
On Tue, Oct 8, 2024 at 10:31 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Oct 8, 2024 at 8:19 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote: > > > We would like to use the high 16bit of the handle_type field to encode > > > file handle traits, such as "connectable". > > > > > > In preparation for this change, make sure that filesystems do not return > > > a handle_type value with upper bits set and that the open_by_handle_at(2) > > > syscall rejects these handle types. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/exportfs/expfs.c | 14 ++++++++++++-- > > > fs/fhandle.c | 6 ++++++ > > > include/linux/exportfs.h | 14 ++++++++++++++ > > > 3 files changed, 32 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c > > > index 4f2dd4ab4486..c8eb660fdde4 100644 > > > --- a/fs/exportfs/expfs.c > > > +++ b/fs/exportfs/expfs.c > > > @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, > > > int *max_len, struct inode *parent, int flags) > > > { > > > const struct export_operations *nop = inode->i_sb->s_export_op; > > > + enum fid_type type; > > > > > > if (!exportfs_can_encode_fh(nop, flags)) > > > return -EOPNOTSUPP; > > > > > > if (!nop && (flags & EXPORT_FH_FID)) > > > - return exportfs_encode_ino64_fid(inode, fid, max_len); > > > + type = exportfs_encode_ino64_fid(inode, fid, max_len); > > > + else > > > + type = nop->encode_fh(inode, fid->raw, max_len, parent); > > > + > > > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(type))) > > > + return -EINVAL; > > > + > > > > The stack trace won't be very useful here. Rather than a WARN, it might > > be better to dump out some info about the fstype (and maybe other > > info?) that returned the bogus type value here. I'm pretty sure most > > in-kernel fs's don't do this, but who knows what 3rd party fs's might > > do. > > > > Right. I changed to: > > if (FILEID_USER_FLAGS(type)) { > pr_warn_once("%s: unexpected fh type value 0x%x from > fstype %s.\n", > __func__, type, inode->i_sb->s_type->name); > return -EINVAL; > } > > FYI, following Jan's comment about mixing bitwise with negative values, I changes this to: if (type > 0 && FILEID_USER_FLAGS(type)) { pr_warn_once("%s: unexpected fh type value 0x%x from fstype %s.\n", __func__, type, inode->i_sb->s_type->name); return -EINVAL; } return type; because ->encode_fh() method are allowed to return a negative error (e.g. -EOVERFLOW) > > > + return type; > > > > > > - return nop->encode_fh(inode, fid->raw, max_len, parent); > > > } > > > EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh); > > > > > > @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len, > > > char nbuf[NAME_MAX+1]; > > > int err; > > > > > > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type))) > > > + return -EINVAL; > > > + > > > > > > This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which > > means that this fh comes from userland or from an NFS client. I don't > > think we want to WARN because someone crafted a bogus fh and passed it > > to us. > > > > Good point, I will remove the WARN and also fix the bug :-/ > > if (FILEID_USER_FLAGS(fileid_type)) > return ERR_PTR(-EINVAL); > And changed this to: @@ -436,6 +446,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len, char nbuf[NAME_MAX+1]; int err; + if (fileid_type < 0 || FILEID_USER_FLAGS(fileid_type)) + return ERR_PTR(-EINVAL); + > Pushed this and othe minor fixes to > https://github.com/amir73il/linux/commits/connectable-fh/ > until we sort out the rest of your comments and maybe get more feedback. > If no further comments I will post v4. Thanks, Amir.
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c index 4f2dd4ab4486..c8eb660fdde4 100644 --- a/fs/exportfs/expfs.c +++ b/fs/exportfs/expfs.c @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len, struct inode *parent, int flags) { const struct export_operations *nop = inode->i_sb->s_export_op; + enum fid_type type; if (!exportfs_can_encode_fh(nop, flags)) return -EOPNOTSUPP; if (!nop && (flags & EXPORT_FH_FID)) - return exportfs_encode_ino64_fid(inode, fid, max_len); + type = exportfs_encode_ino64_fid(inode, fid, max_len); + else + type = nop->encode_fh(inode, fid->raw, max_len, parent); + + if (WARN_ON_ONCE(FILEID_USER_FLAGS(type))) + return -EINVAL; + + return type; - return nop->encode_fh(inode, fid->raw, max_len, parent); } EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh); @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len, char nbuf[NAME_MAX+1]; int err; + if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type))) + return -EINVAL; + /* * Try to get any dentry for the given file handle from the filesystem. */ diff --git a/fs/fhandle.c b/fs/fhandle.c index 82df28d45cd7..c5792cf3c6e9 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -307,6 +307,10 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, retval = -EINVAL; goto out_path; } + if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) { + retval = -EINVAL; + goto out_path; + } handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes), GFP_KERNEL); if (!handle) { @@ -322,6 +326,8 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, goto out_handle; } + /* Filesystem code should not be exposed to user flags */ + handle->handle_type &= ~FILEID_USER_FLAGS_MASK; retval = do_handle_to_path(handle, path, &ctx); out_handle: diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h index 893a1d21dc1c..76a3050b3593 100644 --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -160,6 +160,20 @@ struct fid { #define EXPORT_FH_FID 0x2 /* File handle may be non-decodeable */ #define EXPORT_FH_DIR_ONLY 0x4 /* Only decode file handle for a directory */ +/* + * Filesystems use only lower 8 bits of file_handle type for fid_type. + * name_to_handle_at() uses upper 16 bits of type as user flags to be + * interpreted by open_by_handle_at(). + */ +#define FILEID_USER_FLAGS_MASK 0xffff0000 +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) + +/* Flags supported in encoded handle_type that is exported to user */ +#define FILEID_VALID_USER_FLAGS (0) + +#define FILEID_USER_TYPE_IS_VALID(type) \ + (!(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)) + /** * struct export_operations - for nfsd to communicate with file systems * @encode_fh: encode a file handle fragment from a dentry
We would like to use the high 16bit of the handle_type field to encode file handle traits, such as "connectable". In preparation for this change, make sure that filesystems do not return a handle_type value with upper bits set and that the open_by_handle_at(2) syscall rejects these handle types. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/exportfs/expfs.c | 14 ++++++++++++-- fs/fhandle.c | 6 ++++++ include/linux/exportfs.h | 14 ++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-)