On Tue, Jan 14, 2020 at 09:10:46AM +0100, Christoph Hellwig wrote:
> Lift the common code to copy the cursor from and to user space into
> xfs_ioc_attr_list. Note that this means we copy in twice now as
> the cursor is in the middle of the conaining structure, but we never
> touch the memory for the original copy. Doing so keeps the cursor
> handling isolated in the common helper.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_ioctl.c | 36 +++++++++++++++---------------------
> fs/xfs/xfs_ioctl.h | 2 +-
> fs/xfs/xfs_ioctl32.c | 19 ++++---------------
> 3 files changed, 20 insertions(+), 37 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 8d7b8ad21d9e..899a3b41fa91 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -356,9 +356,10 @@ xfs_ioc_attr_list(
> void __user *ubuf,
> int bufsize,
> int flags,
> - struct attrlist_cursor_kern *cursor)
> + struct xfs_attrlist_cursor __user *ucursor)
> {
> struct xfs_attr_list_context context;
> + struct attrlist_cursor_kern cursor;
> struct xfs_attrlist *alist;
> void *buffer;
> int error;
> @@ -376,10 +377,12 @@ xfs_ioc_attr_list(
> /*
> * Validate the cursor.
> */
> - if (cursor->pad1 || cursor->pad2)
> + if (copy_from_user(&cursor, ucursor, sizeof(cursor)))
> + return -EFAULT;
> + if (cursor.pad1 || cursor.pad2)
> return -EINVAL;
> - if ((cursor->initted == 0) &&
> - (cursor->hashval || cursor->blkno || cursor->offset))
> + if ((cursor.initted == 0) &&
> + (cursor.hashval || cursor.blkno || cursor.offset))
> return -EINVAL;
>
> buffer = kmem_zalloc_large(bufsize, 0);
> @@ -391,7 +394,7 @@ xfs_ioc_attr_list(
> */
> memset(&context, 0, sizeof(context));
> context.dp = dp;
> - context.cursor = cursor;
> + context.cursor = &cursor;
> context.resynch = 1;
> context.flags = flags;
> context.buffer = buffer;
> @@ -408,7 +411,8 @@ xfs_ioc_attr_list(
> if (error)
> goto out_free;
>
> - if (copy_to_user(ubuf, buffer, bufsize))
> + if (copy_to_user(ubuf, buffer, bufsize) ||
> + copy_to_user(ucursor, &cursor, sizeof(cursor)))
> error = -EFAULT;
> out_free:
> kmem_free(buffer);
> @@ -418,33 +422,23 @@ xfs_ioc_attr_list(
> STATIC int
> xfs_attrlist_by_handle(
> struct file *parfilp,
> - void __user *arg)
> + struct xfs_fsop_attrlist_handlereq __user *p)
> {
> - int error = -ENOMEM;
> - attrlist_cursor_kern_t *cursor;
> - struct xfs_fsop_attrlist_handlereq __user *p = arg;
> - xfs_fsop_attrlist_handlereq_t al_hreq;
> + struct xfs_fsop_attrlist_handlereq al_hreq;
> struct dentry *dentry;
> + int error = -ENOMEM;
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> - if (copy_from_user(&al_hreq, arg, sizeof(xfs_fsop_attrlist_handlereq_t)))
> + if (copy_from_user(&al_hreq, p, sizeof(al_hreq)))
> return -EFAULT;
>
> dentry = xfs_handlereq_to_dentry(parfilp, &al_hreq.hreq);
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
>
> - cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
> error = xfs_ioc_attr_list(XFS_I(d_inode(dentry)), al_hreq.buffer,
> - al_hreq.buflen, al_hreq.flags, cursor);
> - if (error)
> - goto out_dput;
> -
> - if (copy_to_user(&p->pos, cursor, sizeof(attrlist_cursor_kern_t)))
> - error = -EFAULT;
> -
> -out_dput:
> + al_hreq.buflen, al_hreq.flags, &p->pos);
> dput(dentry);
> return error;
> }
> diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
> index ec6448b259fb..d6e8000ad825 100644
> --- a/fs/xfs/xfs_ioctl.h
> +++ b/fs/xfs/xfs_ioctl.h
> @@ -40,7 +40,7 @@ int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
> uint32_t opcode, void __user *uname, void __user *value,
> uint32_t *len, uint32_t flags);
> int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf, int bufsize,
> - int flags, struct attrlist_cursor_kern *cursor);
> + int flags, struct xfs_attrlist_cursor __user *ucursor);
>
> extern struct dentry *
> xfs_handle_to_dentry(
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 17e14916757b..c1771e728117 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -352,35 +352,24 @@ xfs_compat_handlereq_to_dentry(
> STATIC int
> xfs_compat_attrlist_by_handle(
> struct file *parfilp,
> - void __user *arg)
> + compat_xfs_fsop_attrlist_handlereq_t __user *p)
> {
> - int error;
> - attrlist_cursor_kern_t *cursor;
> - compat_xfs_fsop_attrlist_handlereq_t __user *p = arg;
> compat_xfs_fsop_attrlist_handlereq_t al_hreq;
> struct dentry *dentry;
> + int error;
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> - if (copy_from_user(&al_hreq, arg,
> - sizeof(compat_xfs_fsop_attrlist_handlereq_t)))
> + if (copy_from_user(&al_hreq, p, sizeof(al_hreq)))
> return -EFAULT;
>
> dentry = xfs_compat_handlereq_to_dentry(parfilp, &al_hreq.hreq);
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
>
> - cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
> error = xfs_ioc_attr_list(XFS_I(d_inode(dentry)),
> compat_ptr(al_hreq.buffer), al_hreq.buflen,
> - al_hreq.flags, cursor);
> - if (error)
> - goto out_dput;
> -
> - if (copy_to_user(&p->pos, cursor, sizeof(attrlist_cursor_kern_t)))
> - error = -EFAULT;
> -
> -out_dput:
> + al_hreq.flags, &p->pos);
> dput(dentry);
> return error;
> }
> --
> 2.24.1
>
