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

Message ID	20200129170310.51370-26-hch@lst.de (mailing list archive)
State	Superseded
Headers	show Return-Path: <SRS0=zOuE=3S=vger.kernel.org=linux-xfs-owner@kernel.org> From: Christoph Hellwig <hch@lst.de> To: linux-xfs@vger.kernel.org Cc: Allison Collins <allison.henderson@oracle.com>, "Darrick J . Wong" <darrick.wong@oracle.com> Subject: [PATCH 25/30] xfs: lift cursor copy in/out into xfs_ioc_attr_list Date: Wed, 29 Jan 2020 18:03:04 +0100 Message-Id: <20200129170310.51370-26-hch@lst.de> In-Reply-To: <20200129170310.51370-1-hch@lst.de> References: <20200129170310.51370-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk
Series	[01/30] xfs: reject invalid flags combinations in XFS_IOC_ATTRLIST_BY_HANDLE \| expand [01/30] xfs: reject invalid flags combinations in XFS_IOC_ATTRLIST_BY_HANDLE [02/30] xfs: remove the ATTR_INCOMPLETE flag [03/30] xfs: merge xfs_attr_remove into xfs_attr_set [04/30] xfs: merge xfs_attrmulti_attr_remove into xfs_attrmulti_attr_set [05/30] xfs: use strndup_user in XFS_IOC_ATTRMULTI_BY_HANDLE [06/30] xfs: factor out a helper for a single XFS_IOC_ATTRMULTI_BY_HANDLE op [07/30] xfs: remove the name == NULL check from xfs_attr_args_init [08/30] xfs: remove the MAXNAMELEN check from xfs_attr_args_init [09/30] xfs: move struct xfs_da_args to xfs_types.h [10/30] xfs: turn xfs_da_args.value into a void pointer [11/30] xfs: pass an initialized xfs_da_args structure to xfs_attr_set [12/30] xfs: pass an initialized xfs_da_args to xfs_attr_get [13/30] xfs: remove the xfs_inode argument to xfs_attr_get_ilocked [14/30] xfs: remove ATTR_KERNOVAL [15/30] xfs: remove ATTR_ALLOC and XFS_DA_OP_ALLOCVAL [16/30] xfs: replace ATTR_KERNOTIME with XFS_DA_OP_NOTIME [17/30] xfs: factor out a xfs_attr_match helper [18/30] xfs: cleanup struct xfs_attr_list_context [19/30] xfs: remove the unused ATTR_ENTRY macro [20/30] xfs: open code ATTR_ENTSIZE [21/30] xfs: move the legacy xfs_attr_list to xfs_ioctl.c [22/30] xfs: rename xfs_attr_list_int to xfs_attr_list [23/30] xfs: lift common checks into xfs_ioc_attr_list [24/30] xfs: lift buffer allocation into xfs_ioc_attr_list [25/30] xfs: lift cursor copy in/out into xfs_ioc_attr_list [26/30] xfs: improve xfs_forget_acl [27/30] xfs: clean up the ATTR_REPLACE checks [28/30] xfs: clean up the attr flag confusion [29/30] xfs: remove XFS_DA_OP_INCOMPLETE [30/30] xfs: embedded the attrlist cursor into struct xfs_attr_list_context

Message ID

20200129170310.51370-26-hch@lst.de (mailing list archive)

State

Superseded

Headers

From: Christoph Hellwig <hch@lst.de>
To: linux-xfs@vger.kernel.org
Cc: Allison Collins <allison.henderson@oracle.com>,
        "Darrick J . Wong" <darrick.wong@oracle.com>
Subject: [PATCH 25/30] xfs: lift cursor copy in/out into xfs_ioc_attr_list
Date: Wed, 29 Jan 2020 18:03:04 +0100
Message-Id: <20200129170310.51370-26-hch@lst.de>
In-Reply-To: <20200129170310.51370-1-hch@lst.de>
References: <20200129170310.51370-1-hch@lst.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-xfs-owner@vger.kernel.org
Precedence: bulk

Series

[01/30] xfs: reject invalid flags combinations in XFS_IOC_ATTRLIST_BY_HANDLE | expand

Commit Message

Christoph Hellwig Jan. 29, 2020, 5:03 p.m. UTC

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@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(-)

Comments

Chandan Rajendra Feb. 10, 2020, 6:02 a.m. UTC | #1

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;
>  }
>

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;
 }

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

Commit Message

Comments

Patch