[PATCH 0/2] pipe: Fixes

[PATCH 0/2] pipe: Fixes

[David Howells]

Hi Linus, Eric,

Here are a couple of patches to fix bugs syzbot found in the pipe changes:

 (1) An assertion check will sometimes trip when polling a pipe because the
     ring size and indices used are approximate and may be being changed
     simultaneously.

     An equivalent approximate calculation was done previously, but without
     the assertion check, so I've just dropped the check.  To make it
     accurate, the pipe mutex would need to be taken or the spin lock could
     be used - but usage of the spinlock would need to be rolled out into
     splice, iov_iter and other places for that.

 (2) The index mask and the max_usage values need to be regenerated after
     dropping the locks to wait in pipe_write() as F_SETPIPE_SZ could have
     been called during the wait.

David
---
David Howells (2):
      pipe: Remove assertion from pipe_poll()
      pipe: Fix missing mask update after pipe_wait()


 fs/pipe.c |    3 +++
 1 file changed, 3 insertions(+)

[PATCH 1/2] pipe: Remove assertion from pipe_poll()

[David Howells]

An assertion check was added to pipe_poll() to make sure that the ring
occupancy isn't seen to overflow the ring size.  However, since no locks
are held when the three values are read, it is possible for F_SETPIPE_SZ to
intervene and muck up the calculation, thereby causing the oops.

Fix this by simply removing the assertion and accepting that the
calculation might be approximate.

Note that the previous code also had a similar issue, though there was no
assertion check, since the occupancy counter and the ring size were not
read with a lock held, so it's possible that the poll check might have
malfunctioned then too.

Also wake up all the waiters so that they can reissue their checks if there
was a competing read or write.

Fixes: 8cefc107ca54 ("pipe: Use head and tail pointers for the ring, not cursor and length")
Reported-by: syzbot+d37abaade33a934f16f2@syzkaller.appspotmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Eric Biggers <ebiggers@kernel.org>
---

 fs/pipe.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/pipe.c b/fs/pipe.c
index 648ce440ca85..5f89f73d4366 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1176,6 +1176,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	pipe->max_usage = nr_slots;
 	pipe->tail = tail;
 	pipe->head = head;
+	wake_up_interruptible_all(&pipe->wait);
 	return pipe->max_usage * PAGE_SIZE;
 
 out_revert_acct:

[PATCH 2/2] pipe: Fix missing mask update after pipe_wait()

[David Howells]

Fix pipe_write() to regenerate the ring index mask and update max_usage
after calling pipe_wait().

This is necessary as the latter function drops the pipe lock, thereby
allowing F_SETPIPE_SZ change it.  Without this, pipe_write() may
subsequently miscalculate the array indices and pipe fullness, leading to
an oops like the following:

 BUG: KASAN: slab-out-of-bounds in pipe_write+0xc25/0xe10 fs/pipe.c:481
 Write of size 8 at addr ffff8880771167a8 by task syz-executor.3/7987
 ...
 CPU: 1 PID: 7987 Comm: syz-executor.3 Not tainted 5.4.0-rc2-syzkaller #0
 ...
 Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x113/0x167 lib/dump_stack.c:113
  print_address_description.constprop.8.cold.10+0x9/0x31d mm/kasan/report.c:374
  __kasan_report.cold.11+0x1b/0x3a mm/kasan/report.c:506
  kasan_report+0x12/0x20 mm/kasan/common.c:634
  __asan_report_store8_noabort+0x17/0x20 mm/kasan/generic_report.c:137
  pipe_write+0xc25/0xe10 fs/pipe.c:481
  call_write_iter include/linux/fs.h:1895 [inline]
  new_sync_write+0x3fd/0x7e0 fs/read_write.c:483
  __vfs_write+0x94/0x110 fs/read_write.c:496
  vfs_write+0x18a/0x520 fs/read_write.c:558
  ksys_write+0x105/0x220 fs/read_write.c:611
  __do_sys_write fs/read_write.c:623 [inline]
  __se_sys_write fs/read_write.c:620 [inline]
  __x64_sys_write+0x6e/0xb0 fs/read_write.c:620
  do_syscall_64+0xca/0x5d0 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Fixes: 8cefc107ca54 ("pipe: Use head and tail pointers for the ring, not cursor and length")
Reported-by: syzbot+838eb0878ffd51f27c41@syzkaller.appspotmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Eric Biggers <ebiggers@kernel.org>
---

 fs/pipe.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pipe.c b/fs/pipe.c
index 5f89f73d4366..4d2a7bbc5d31 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -526,6 +526,8 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 		}
 		pipe->waiting_writers++;
 		pipe_wait(pipe);
+		mask = pipe->ring_size - 1;
+		max_usage = pipe->max_usage;
 		pipe->waiting_writers--;
 	}
 out:

Re: [PATCH 2/2] pipe: Fix missing mask update after pipe_wait()

[Linus Torvalds]

On Thu, Dec 5, 2019 at 9:22 AM David Howells <dhowells@redhat.com> wrote:
>
> Fix pipe_write() to regenerate the ring index mask and update max_usage
> after calling pipe_wait().

Honestly, just remove the "mask" and "max_usage" caching. There are no
advantages to it. With all the function calls etc, it will just result
in moving the data from the pipe to a stack slot anyway.

Maybe you can cache it inside the inner loops or something, but
caching it at the outer level is pointless, and leads to these kinds
of bugs.

               Linus

Re: [PATCH 1/2] pipe: Remove assertion from pipe_poll()

[Linus Torvalds]

On Thu, Dec 5, 2019 at 9:21 AM David Howells <dhowells@redhat.com> wrote:
>
> An assertion check was added to pipe_poll() to make sure that the ring
> occupancy isn't seen to overflow the ring size.  However, since no locks
> are held when the three values are read, it is possible for F_SETPIPE_SZ to
> intervene and muck up the calculation, thereby causing the oops.
>
> Fix this by simply removing the assertion and accepting that the
> calculation might be approximate.

This is not what the patch actually does.

The patch you sent only adds the wakeup when the pipe size changes.

Please re-generate both patches and re-send.

              Linus