[v6,04/11] btrfs: fix btrfs_write_check()

Commit Message

Omar Sandoval Nov. 18, 2020, 7:18 p.m. UTC

From: Omar Sandoval <[e-mail]>

btrfs_write_check() has two related bugs:

1. It gets the iov_iter count before calling generic_write_checks(), but
   generic_write_checks() may truncate the iov_iter.
2. It returns the count or negative errno as a size_t, which the callers
   cast to an int. If the count is greater than INT_MAX, this overflows.

To fix both of these, pull the call to generic_write_checks() out of
btrfs_write_check(), use the new iov_iter count returned from
generic_write_checks(), and have btrfs_write_check() return 0 or a
negative errno as an int instead of the count. This rearrangement also
paves the way for RWF_ENCODED write support.

Fixes: f945968ff64c ("btrfs: introduce btrfs_write_check()")
Signed-off-by: Omar Sandoval <[e-mail]>
---
 fs/btrfs/file.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

David Sterba Nov. 23, 2020, 5:08 p.m. UTC | #1

On Wed, Nov 18, 2020 at 11:18:11AM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <[e-mail]>
> 
> btrfs_write_check() has two related bugs:
> 
> 1. It gets the iov_iter count before calling generic_write_checks(), but
>    generic_write_checks() may truncate the iov_iter.
> 2. It returns the count or negative errno as a size_t, which the callers
>    cast to an int. If the count is greater than INT_MAX, this overflows.
> 
> To fix both of these, pull the call to generic_write_checks() out of
> btrfs_write_check(), use the new iov_iter count returned from
> generic_write_checks(), and have btrfs_write_check() return 0 or a
> negative errno as an int instead of the count. This rearrangement also
> paves the way for RWF_ENCODED write support.
> 
> Fixes: f945968ff64c ("btrfs: introduce btrfs_write_check()")

This patch is still in misc-next and the commit id is unstable, so this
would rather be folded to the patch.

Omar Sandoval Nov. 30, 2020, 7:18 p.m. UTC | #2

On Mon, Nov 23, 2020 at 06:08:31PM +0100, David Sterba wrote:
> On Wed, Nov 18, 2020 at 11:18:11AM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval <[e-mail]>
> > 
> > btrfs_write_check() has two related bugs:
> > 
> > 1. It gets the iov_iter count before calling generic_write_checks(), but
> >    generic_write_checks() may truncate the iov_iter.
> > 2. It returns the count or negative errno as a size_t, which the callers
> >    cast to an int. If the count is greater than INT_MAX, this overflows.
> > 
> > To fix both of these, pull the call to generic_write_checks() out of
> > btrfs_write_check(), use the new iov_iter count returned from
> > generic_write_checks(), and have btrfs_write_check() return 0 or a
> > negative errno as an int instead of the count. This rearrangement also
> > paves the way for RWF_ENCODED write support.
> > 
> > Fixes: f945968ff64c ("btrfs: introduce btrfs_write_check()")
> 
> This patch is still in misc-next and the commit id is unstable, so this
> would rather be folded to the patch.

Looks like you folded this in on misc-next, thanks!

Patch

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index d217b739b164..7225b63b62a9 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1583,21 +1583,17 @@  static void update_time_for_write(struct inode *inode)
 		inode_inc_iversion(inode);
 }
 
-static size_t btrfs_write_check(struct kiocb *iocb, struct iov_iter *from)
+static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from,
+			     size_t count)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	loff_t pos = iocb->ki_pos;
-	size_t count = iov_iter_count(from);
 	int err;
 	loff_t oldsize;
 	loff_t start_pos;
 
-	err = generic_write_checks(iocb, from);
-	if (err <= 0)
-		return err;
-
 	if (iocb->ki_flags & IOCB_NOWAIT) {
 		size_t nocow_bytes = count;
 
@@ -1639,7 +1635,7 @@  static size_t btrfs_write_check(struct kiocb *iocb, struct iov_iter *from)
 		}
 	}
 
-	return count;
+	return 0;
 }
 
 static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
@@ -1656,7 +1652,7 @@  static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 	u64 lockend;
 	size_t num_written = 0;
 	int nrptrs;
-	int ret = 0;
+	ssize_t ret;
 	bool only_release_metadata = false;
 	bool force_page_uptodate = false;
 	loff_t old_isize = i_size_read(inode);
@@ -1669,10 +1665,14 @@  static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 	if (ret < 0)
 		return ret;
 
-	ret = btrfs_write_check(iocb, i);
+	ret = generic_write_checks(iocb, i);
 	if (ret <= 0)
 		goto out;
 
+	ret = btrfs_write_check(iocb, i, ret);
+	if (ret < 0)
+		goto out;
+
 	pos = iocb->ki_pos;
 	nrptrs = min(DIV_ROUND_UP(iov_iter_count(i), PAGE_SIZE),
 			PAGE_SIZE / (sizeof(struct page *)));
@@ -1904,7 +1904,7 @@  static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t written = 0;
 	ssize_t written_buffered;
 	loff_t endbyte;
-	int err;
+	ssize_t err;
 	unsigned int ilock_flags = 0;
 	struct iomap_dio *dio = NULL;
 
@@ -1920,8 +1920,14 @@  static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	if (err < 0)
 		return err;
 
-	err = btrfs_write_check(iocb, from);
+	err = generic_write_checks(iocb, from);
 	if (err <= 0) {
+		btrfs_inode_unlock(inode, ilock_flags);
+		return err;
+	}
+
+	err = btrfs_write_check(iocb, from, err);
+	if (err < 0) {
 		btrfs_inode_unlock(inode, ilock_flags);
 		goto out;
 	}