[PATCH v2 0/2] iter revert problems

[PATCH v2 0/2] iter revert problems

[Pavel Begunkov]

For the bug description see 2/2. As mentioned there the current problems
is because of generic_write_checks(), but there was also a similar case
fixed in 5.12, which should have been triggerable by normal
write(2)/read(2) and others.

It may be better to enforce reexpands as a long term solution, but for
now this patchset is quickier and easier to backport.

v2: don't fail it has been justly fully reverted

Pavel Begunkov (2):
  iov_iter: mark truncated iters
  io_uring: don't retry with truncated iter

 fs/io_uring.c       | 16 ++++++++++++++++
 include/linux/uio.h |  5 ++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.32.0

[PATCH v2 1/2] iov_iter: mark truncated iters

[Pavel Begunkov]

Track if an iterator has ever been truncated. This will be used to
mitigate revert-truncate problems.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/linux/uio.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 82c3c3e819e0..61b8d312d13a 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -30,6 +30,7 @@ enum iter_type {
 struct iov_iter {
 	u8 iter_type;
 	bool data_source;
+	bool truncated;
 	size_t iov_offset;
 	size_t count;
 	union {
@@ -254,8 +255,10 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
 	 * conversion in assignement is by definition greater than all
 	 * values of size_t, including old i->count.
 	 */
-	if (i->count > count)
+	if (i->count > count) {
 		i->count = count;
+		i->truncated = true;
+	}
 }
 
 /*
-- 
2.32.0

[PATCH v2 2/2] io_uring: don't retry with truncated iter

[Pavel Begunkov]

[   74.211232] BUG: KASAN: stack-out-of-bounds in iov_iter_revert+0x809/0x900
[   74.212778] Read of size 8 at addr ffff888025dc78b8 by task
syz-executor.0/828
[   74.214756] CPU: 0 PID: 828 Comm: syz-executor.0 Not tainted
5.14.0-rc3-next-20210730 #1
[   74.216525] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[   74.219033] Call Trace:
[   74.219683]  dump_stack_lvl+0x8b/0xb3
[   74.220706]  print_address_description.constprop.0+0x1f/0x140
[   74.224226]  kasan_report.cold+0x7f/0x11b
[   74.226085]  iov_iter_revert+0x809/0x900
[   74.227960]  io_write+0x57d/0xe40
[   74.232647]  io_issue_sqe+0x4da/0x6a80
[   74.242578]  __io_queue_sqe+0x1ac/0xe60
[   74.245358]  io_submit_sqes+0x3f6e/0x76a0
[   74.248207]  __do_sys_io_uring_enter+0x90c/0x1a20
[   74.257167]  do_syscall_64+0x3b/0x90
[   74.257984]  entry_SYSCALL_64_after_hwframe+0x44/0xae

old_size = iov_iter_count();
...
iov_iter_revert(old_size - iov_iter_count());

If iov_iter_revert() is done base on the initial size as above, and the
iter is truncated and not reexpanded in the middle, it miscalculates
borders causing problems. This trace is due to no one reexpanding after
generic_write_checks().

Avoid reverting truncated iterators, so io_uring would fail requests
with EAGAIN instead of retrying them.

Cc: stable@vger.kernel.org
Reported-by: Palash Oswal <oswalpalash@gmail.com>
Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Reported-and-tested-by: syzbot+9671693590ef5aad8953@syzkaller.appspotmail.com
Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index efd818419014..2e168051262d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2462,6 +2462,16 @@ static void kiocb_end_write(struct io_kiocb *req)
 	}
 }
 
+static inline bool io_check_truncated(struct iov_iter *i, size_t len)
+{
+	if (unlikely(i->truncated)) {
+		if (iov_iter_count(i) != len)
+			return false;
+		i->truncated = false;
+	}
+	return true;
+}
+
 #ifdef CONFIG_BLOCK
 static bool io_resubmit_prep(struct io_kiocb *req)
 {
@@ -2469,6 +2479,8 @@ static bool io_resubmit_prep(struct io_kiocb *req)
 
 	if (!rw)
 		return !io_req_prep_async(req);
+	if (!io_check_truncated(&rw->iter, req->result))
+		return false;
 	/* may have left rw->iter inconsistent on -EIOCBQUEUED */
 	iov_iter_revert(&rw->iter, req->result - iov_iter_count(&rw->iter));
 	return true;
@@ -3328,6 +3340,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 		/* no retry on NONBLOCK nor RWF_NOWAIT */
 		if (req->flags & REQ_F_NOWAIT)
 			goto done;
+		if (!io_check_truncated(iter, io_size))
+			goto done;
 		/* some cases will consume bytes even on error returns */
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = 0;
@@ -3467,6 +3481,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		kiocb_done(kiocb, ret2, issue_flags);
 	} else {
 copy_iov:
+		if (!io_check_truncated(iter, io_size))
+			goto done;
 		/* some cases will consume bytes even on error returns */
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
-- 
2.32.0

Re: [PATCH v2 0/2] iter revert problems

[Jens Axboe]

On 8/12/21 2:40 PM, Pavel Begunkov wrote:
> For the bug description see 2/2. As mentioned there the current problems
> is because of generic_write_checks(), but there was also a similar case
> fixed in 5.12, which should have been triggerable by normal
> write(2)/read(2) and others.
> 
> It may be better to enforce reexpands as a long term solution, but for
> now this patchset is quickier and easier to backport.
> 
> v2: don't fail it has been justly fully reverted

Al, what do you think of this approach? It'll fix the issue, but might be
cleaner to have iov->truncated actually track the truncated size. That'd
make it a more complete solution, at the expense of bloat iov_iter which
this version will not.

-- 
Jens Axboe

Re: [PATCH v2 0/2] iter revert problems

[Pavel Begunkov]

On 8/12/21 9:40 PM, Pavel Begunkov wrote:
> For the bug description see 2/2. As mentioned there the current problems
> is because of generic_write_checks(), but there was also a similar case
> fixed in 5.12, which should have been triggerable by normal
> write(2)/read(2) and others.
> 
> It may be better to enforce reexpands as a long term solution, but for
> now this patchset is quickier and easier to backport.

We need to do something with this, hopefully soon.


> v2: don't fail it has been justly fully reverted
> 
> Pavel Begunkov (2):
>   iov_iter: mark truncated iters
>   io_uring: don't retry with truncated iter
> 
>  fs/io_uring.c       | 16 ++++++++++++++++
>  include/linux/uio.h |  5 ++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 

-- 
Pavel Begunkov

Re: [PATCH v2 0/2] iter revert problems

[Al Viro]

On Sat, Aug 21, 2021 at 03:24:28PM +0100, Pavel Begunkov wrote:
> On 8/12/21 9:40 PM, Pavel Begunkov wrote:
> > For the bug description see 2/2. As mentioned there the current problems
> > is because of generic_write_checks(), but there was also a similar case
> > fixed in 5.12, which should have been triggerable by normal
> > write(2)/read(2) and others.
> > 
> > It may be better to enforce reexpands as a long term solution, but for
> > now this patchset is quickier and easier to backport.
> 
> We need to do something with this, hopefully soon.

I still don't like that approach ;-/  If anything, I would rather do
something like this, and to hell with one extra word on stack in
several functions; at least that way the semantics is easy to describe.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/io_uring.c b/fs/io_uring.c
index d94fb5835a20..5501f8b3af3b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3420,6 +3420,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	} else {
 copy_iov:
 		/* some cases will consume bytes even on error returns */
+		iov_iter_reexpand(iter, iter->count + iter->truncated);
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
 		return ret ?: -EAGAIN;
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 82c3c3e819e0..5265024e8b90 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -47,6 +47,7 @@ struct iov_iter {
 		};
 		loff_t xarray_start;
 	};
+	size_t truncated;
 };
 
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
@@ -254,8 +255,10 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
 	 * conversion in assignement is by definition greater than all
 	 * values of size_t, including old i->count.
 	 */
-	if (i->count > count)
+	if (i->count > count) {
+		i->truncated += i->count - count;
 		i->count = count;
+	}
 }
 
 /*
@@ -264,6 +267,7 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
  */
 static inline void iov_iter_reexpand(struct iov_iter *i, size_t count)
 {
+	i->truncated -= count - i->count;
 	i->count = count;
 }
 

Re: [PATCH v2 0/2] iter revert problems

[Jens Axboe]

On 8/21/21 4:25 PM, Al Viro wrote:
> On Sat, Aug 21, 2021 at 03:24:28PM +0100, Pavel Begunkov wrote:
>> On 8/12/21 9:40 PM, Pavel Begunkov wrote:
>>> For the bug description see 2/2. As mentioned there the current problems
>>> is because of generic_write_checks(), but there was also a similar case
>>> fixed in 5.12, which should have been triggerable by normal
>>> write(2)/read(2) and others.
>>>
>>> It may be better to enforce reexpands as a long term solution, but for
>>> now this patchset is quickier and easier to backport.
>>
>> We need to do something with this, hopefully soon.
> 
> I still don't like that approach ;-/  If anything, I would rather do
> something like this, and to hell with one extra word on stack in
> several functions; at least that way the semantics is easy to describe.

Pavel suggested this very approach initially as well when we discussed
it, and if you're fine with the extra size_t, it is by far the best way
to get this done and not have a wonky/fragile API.

-- 
Jens Axboe

RE: [PATCH v2 0/2] iter revert problems

[David Laight]

From: Jens Axboe
> Sent: 22 August 2021 00:14
> 
> On 8/21/21 4:25 PM, Al Viro wrote:
> > On Sat, Aug 21, 2021 at 03:24:28PM +0100, Pavel Begunkov wrote:
> >> On 8/12/21 9:40 PM, Pavel Begunkov wrote:
> >>> For the bug description see 2/2. As mentioned there the current problems
> >>> is because of generic_write_checks(), but there was also a similar case
> >>> fixed in 5.12, which should have been triggerable by normal
> >>> write(2)/read(2) and others.
> >>>
> >>> It may be better to enforce reexpands as a long term solution, but for
> >>> now this patchset is quickier and easier to backport.
> >>
> >> We need to do something with this, hopefully soon.
> >
> > I still don't like that approach ;-/  If anything, I would rather do
> > something like this, and to hell with one extra word on stack in
> > several functions; at least that way the semantics is easy to describe.
> 
> Pavel suggested this very approach initially as well when we discussed
> it, and if you're fine with the extra size_t, it is by far the best way
> to get this done and not have a wonky/fragile API.

All (well maybe almost all) the users of iov_iter have the
short iov[] cache and the pointer to the big iov[] to kfree()
allocated together with the iov_iter structure itself.
These are almost always on stack.

Putting the whole lot together in a single structure would
make the call sequences a lot less complex and wouldn't use
any more stack/data is almost all the cases.

It would also mean that the 'iter' code could always have a pointer
to the base of the original iov[] list.
The lack of which is probably makes the 'revert' code hard?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)