[v3,08/13] vfs: copy_file_range needs to strip setuid bits and update timestamps
diff mbox series

Message ID 20190529174318.22424-9-amir73il@gmail.com
State New
Headers show
Series
  • Fixes for major copy_file_range() issues
Related show

Commit Message

Amir Goldstein May 29, 2019, 5:43 p.m. UTC
Because generic_copy_file_range doesn't hold the destination inode lock
throughout the copy, strip setuid bits before and after copy.

The destination inode mtime is updated before and after the copy and the
source inode atime is updated after the copy, similar to
generic_file_read_iter().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/read_write.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Darrick J. Wong May 29, 2019, 6:33 p.m. UTC | #1
On Wed, May 29, 2019 at 08:43:12PM +0300, Amir Goldstein wrote:
> Because generic_copy_file_range doesn't hold the destination inode lock
> throughout the copy, strip setuid bits before and after copy.
> 
> The destination inode mtime is updated before and after the copy and the
> source inode atime is updated after the copy, similar to
> generic_file_read_iter().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks reasonable,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/read_write.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index cec7e7b1f693..706ea5f276a7 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1590,8 +1590,27 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
>  				struct file *file_out, loff_t pos_out,
>  				size_t len, unsigned int flags)
>  {
> -	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> -				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
> +	struct inode *inode_out = file_inode(file_out);
> +	int ret, err;
> +
> +	/* Should inode_out lock be held throughout the copy operation? */
> +	inode_lock(inode_out);
> +	err = file_modified(file_out);
> +	inode_unlock(inode_out);
> +	if (err)
> +		return err;
> +
> +	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> +			       len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
> +
> +	file_accessed(file_in);
> +
> +	/* To be on the safe side, remove privs also after copy */
> +	inode_lock(inode_out);
> +	err = file_modified(file_out);
> +	inode_unlock(inode_out);
> +
> +	return err ?: ret;
>  }
>  EXPORT_SYMBOL(generic_copy_file_range);
>  
> -- 
> 2.17.1
>
Amir Goldstein May 29, 2019, 9:08 p.m. UTC | #2
On Wed, May 29, 2019 at 9:33 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, May 29, 2019 at 08:43:12PM +0300, Amir Goldstein wrote:
> > Because generic_copy_file_range doesn't hold the destination inode lock
> > throughout the copy, strip setuid bits before and after copy.
> >
> > The destination inode mtime is updated before and after the copy and the
> > source inode atime is updated after the copy, similar to
> > generic_file_read_iter().
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks reasonable,

Actually, it isn't reasonable. I'd like to recall this patch :-/
As one might expect, splice_direct_to_actor() already has file_accessed()
and "file_modified" is the responsibility of filesystem's ->write_iter().

Thanks,
Amir.

Patch
diff mbox series

diff --git a/fs/read_write.c b/fs/read_write.c
index cec7e7b1f693..706ea5f276a7 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1590,8 +1590,27 @@  ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
 				size_t len, unsigned int flags)
 {
-	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
-				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
+	struct inode *inode_out = file_inode(file_out);
+	int ret, err;
+
+	/* Should inode_out lock be held throughout the copy operation? */
+	inode_lock(inode_out);
+	err = file_modified(file_out);
+	inode_unlock(inode_out);
+	if (err)
+		return err;
+
+	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
+			       len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
+
+	file_accessed(file_in);
+
+	/* To be on the safe side, remove privs also after copy */
+	inode_lock(inode_out);
+	err = file_modified(file_out);
+	inode_unlock(inode_out);
+
+	return err ?: ret;
 }
 EXPORT_SYMBOL(generic_copy_file_range);