linux-kernel - Re: 4.9.0 regression in pipe-backed iov

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Jan 2017 11:18:42 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     "Alan J. Wylie" <alan@...ie.me.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: 4.9.0 regression in pipe-backed iov_iter with systemd-nspawn

On Fri, Jan 13, 2017 at 10:20:19AM +0000, Al Viro wrote:

> OK, so it is iov_iter_advance() failing to free the shit allocated, either
> due to some breakage in pipe_advance() or buggered 'copied'...  Let's
> see which one; could you apply the following and run your reproducer?  The
> only difference from the previous is that it collects and prints a bit more,
> so it should be just as reproducible...

Actually, I think I understand what's going on.  It's
        if (pipe->nrbufs) {
                int unused = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);
                /* [curbuf,unused) is in use.  Free [idx,unused) */
                while (idx != unused) {
                        pipe_buf_release(pipe, &pipe->bufs[idx]);
                        idx = next_idx(idx, pipe);
                        pipe->nrbufs--;
                }
        }
in case when pipe->nrbufs == pipe->buffers and idx == pipe->curbuf.  IOW,
we have a full pipe and want to empty it entirely; the fun question is,
of course, telling that case from having nothing to free with the same
full pipe...

OK, so we have either
	* off != 0 => something's being left in the pipe, i->idx points
to the last in-use buffer after that, idx points to the first buffer unused
after that.  In that case the current logics is correct.
	* off == 0 => we are emptying the damn thing.  i->idx and idx point
to the first buffer unused after that (== pipe->curbuf + pipe->nrbufs at the
time we'd set the iov_iter up).  The current logics is correct unless
pipe->nrbufs was originally 0 and now has become pipe->buffers.  IOW,
we screw up when off == 0, idx == unused, pipe->nrbufs == pipe->buffers...

	OK, we really ought to make sure that iov_iter_pipe() is never
done on a full pipe.  AFAICS, we do, and if so the following should
suffice.  WARNING: it's completely untested and it's a result of debugging
a fencepost bug in handling of cyclic buffers done at 6 in the morning,
on _way_ too long uptime.  Treat as very dangerous; it's not entirely
impossible that I hadn't fucked up, but don't consider it anything other
than "let's try and see if it immediately explodes" until I've got
a chance to reread it after getting some sleep.

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 25f572303801..7bc0b99d3c83 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -759,11 +759,12 @@ static void pipe_advance(struct iov_iter *i, size_t size)
 		idx = next_idx(idx, pipe);
 	if (pipe->nrbufs) {
 		int unused = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);
-		/* [curbuf,unused) is in use.  Free [idx,unused) */
-		while (idx != unused) {
-			pipe_buf_release(pipe, &pipe->bufs[idx]);
-			idx = next_idx(idx, pipe);
-			pipe->nrbufs--;
+		if (idx != unused || unlikely(idx == pipe->curbuf && !off)) {
+			do {
+				pipe_buf_release(pipe, &pipe->bufs[idx]);
+				idx = next_idx(idx, pipe);
+				pipe->nrbufs--;
+			} while (idx != unused);
 		}
 	}
 	i->count -= orig_sz;
@@ -826,6 +827,7 @@ void iov_iter_pipe(struct iov_iter *i, int direction,
 			size_t count)
 {
 	BUG_ON(direction != ITER_PIPE);
+	WARN_ON(pipe->nrbufs == pipe->buffers);
 	i->type = direction;
 	i->pipe = pipe;
 	i->idx = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);