Re: [PATCH 25/30] xfs: lift cursor copy in/out into xfs_ioc_attr_list

[Chandan Rajendra <chandan@linux.ibm.com>]

On Wednesday, January 29, 2020 10:33 PM 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.
>

Logically, code flow remains the same.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  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 cdb3800dfcef..5d160e82778e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -354,9 +354,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;
>  }
> 


-- 
chandan