| Message ID | 20190529174318.22424-6-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for major copy_file_range() issues | expand |
On Wed, May 29, 2019 at 08:43:09PM +0300, Amir Goldstein wrote: > Like the clone and dedupe interfaces we've recently fixed, the > copy_file_range() implementation is missing basic sanity, limits and > boundary condition tests on the parameters that are passed to it > from userspace. Create a new "generic_copy_file_checks()" function > modelled on the generic_remap_checks() function to provide this > missing functionality. > > [Amir] Shorten copy length instead of checking pos_in limits > because input file size already abides by the limits. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/read_write.c | 3 ++- > include/linux/fs.h | 3 +++ > mm/filemap.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/fs/read_write.c b/fs/read_write.c > index f1900bdb3127..b0fb1176b628 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -1626,7 +1626,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > return -EXDEV; > > - ret = generic_file_rw_checks(file_in, file_out); > + ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, > + flags); > if (unlikely(ret)) > return ret; > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 89b9b73eb581..e4d382c4342a 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -3050,6 +3050,9 @@ extern int generic_remap_checks(struct file *file_in, loff_t pos_in, > struct file *file_out, loff_t pos_out, > loff_t *count, unsigned int remap_flags); > extern int generic_file_rw_checks(struct file *file_in, struct file *file_out); > +extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > + struct file *file_out, loff_t pos_out, > + size_t *count, unsigned int flags); > extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *); > extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *); > extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *); > diff --git a/mm/filemap.c b/mm/filemap.c > index 44361928bbb0..aac71aef4c61 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -3056,6 +3056,59 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out) > return 0; > } > > +/* > + * Performs necessary checks before doing a file copy > + * > + * Can adjust amount of bytes to copy via @req_count argument. > + * Returns appropriate error code that caller should return or > + * zero in case the copy should be allowed. > + */ > +int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > + struct file *file_out, loff_t pos_out, > + size_t *req_count, unsigned int flags) > +{ > + struct inode *inode_in = file_inode(file_in); > + struct inode *inode_out = file_inode(file_out); > + uint64_t count = *req_count; > + loff_t size_in; > + int ret; > + > + ret = generic_file_rw_checks(file_in, file_out); > + if (ret) > + return ret; > + > + /* Don't touch certain kinds of inodes */ > + if (IS_IMMUTABLE(inode_out)) > + return -EPERM; > + > + if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) > + return -ETXTBSY; > + > + /* Ensure offsets don't wrap. */ > + if (pos_in + count < pos_in || pos_out + count < pos_out) > + return -EOVERFLOW; > + > + /* Shorten the copy to EOF */ > + size_in = i_size_read(inode_in); > + if (pos_in >= size_in) > + count = 0; > + else > + count = min(count, size_in - (uint64_t)pos_in); > + > + ret = generic_write_check_limits(file_out, pos_out, &count); > + if (ret) > + return ret; > + > + /* Don't allow overlapped copying within the same file. */ > + if (inode_in == inode_out && > + pos_out + count > pos_in && > + pos_out < pos_in + count) > + return -EINVAL; > + > + *req_count = count; > + return 0; > +} > + > int pagecache_write_begin(struct file *file, struct address_space *mapping, > loff_t pos, unsigned len, unsigned flags, > struct page **pagep, void **fsdata) > -- > 2.17.1 >
diff --git a/fs/read_write.c b/fs/read_write.c index f1900bdb3127..b0fb1176b628 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1626,7 +1626,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) return -EXDEV; - ret = generic_file_rw_checks(file_in, file_out); + ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, + flags); if (unlikely(ret)) return ret; diff --git a/include/linux/fs.h b/include/linux/fs.h index 89b9b73eb581..e4d382c4342a 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3050,6 +3050,9 @@ extern int generic_remap_checks(struct file *file_in, loff_t pos_in, struct file *file_out, loff_t pos_out, loff_t *count, unsigned int remap_flags); extern int generic_file_rw_checks(struct file *file_in, struct file *file_out); +extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in, + struct file *file_out, loff_t pos_out, + size_t *count, unsigned int flags); extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *); extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *); extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *); diff --git a/mm/filemap.c b/mm/filemap.c index 44361928bbb0..aac71aef4c61 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3056,6 +3056,59 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out) return 0; } +/* + * Performs necessary checks before doing a file copy + * + * Can adjust amount of bytes to copy via @req_count argument. + * Returns appropriate error code that caller should return or + * zero in case the copy should be allowed. + */ +int generic_copy_file_checks(struct file *file_in, loff_t pos_in, + struct file *file_out, loff_t pos_out, + size_t *req_count, unsigned int flags) +{ + struct inode *inode_in = file_inode(file_in); + struct inode *inode_out = file_inode(file_out); + uint64_t count = *req_count; + loff_t size_in; + int ret; + + ret = generic_file_rw_checks(file_in, file_out); + if (ret) + return ret; + + /* Don't touch certain kinds of inodes */ + if (IS_IMMUTABLE(inode_out)) + return -EPERM; + + if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) + return -ETXTBSY; + + /* Ensure offsets don't wrap. */ + if (pos_in + count < pos_in || pos_out + count < pos_out) + return -EOVERFLOW; + + /* Shorten the copy to EOF */ + size_in = i_size_read(inode_in); + if (pos_in >= size_in) + count = 0; + else + count = min(count, size_in - (uint64_t)pos_in); + + ret = generic_write_check_limits(file_out, pos_out, &count); + if (ret) + return ret; + + /* Don't allow overlapped copying within the same file. */ + if (inode_in == inode_out && + pos_out + count > pos_in && + pos_out < pos_in + count) + return -EINVAL; + + *req_count = count; + return 0; +} + int pagecache_write_begin(struct file *file, struct address_space *mapping, loff_t pos, unsigned len, unsigned flags, struct page **pagep, void **fsdata)