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 16:59:37 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     "Alan J. Wylie" <alan@...ie.me.uk>,
        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 2:50 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> Or, even better, we can get rid of all wraparound-related crap if we
> calculate the final value of pipe->nrbufs and watch for _that_ as
> loop condition:

Ok, I like the 'expected nrbufs' approach. It makes the actual freeing
loop trivial, and gets rid of that whole complexity.

I'm not totally enamored with the details of exactly how that expected
valie is calculated, though.

> +static inline void pipe_truncate(struct iov_iter *i)
> +{
> +       struct pipe_inode_info *pipe = i->pipe;
> +       if (pipe->nrbufs) {
> +               size_t off = i->iov_offset;
> +               int idx = i->idx;
> +               int nrbufs = (idx - pipe->curbuf) & (pipe->buffers - 1);

Ok, here we have what superficially appears to be the same wraparound
issue, except not.

The idx/iov_offset rules are subtly different from the pipe buffer
index rules, and point to the actual last buffer.

So here, 'idx' really points to the last buffer, and we're possibly
off by one, and then you fix that up with:

> +               if (off) {
> +                       pipe->bufs[idx].len = off - pipe->bufs[idx].offset;
> +                       idx = next_idx(idx, pipe);
> +                       nrbufs++;
> +               }

and I think this all even works, and I like how the logic here matches
what the PIPE_PARANOIA tests are. Fine.

EXCEPT.

I don't think "i->iov_offset" is actually correct. If you truncated
the whole thing, you should have cleared iov_offset too, and that
never happened. So truncating everything will not empty the buffers
for us, we'll still get to that "if (off)" case and have nrbufs=1.

So I'd expect something like

    if (!size)
        i->iov_offset = 0;

in pipe_advance(), in order to really free all buffers for that case. No?

Or is there some guarantee that iov_offset was already zero there? I
don't see it, but I'm batting zero on this code...

So I'm clearly still missing something. It looks better to me, but I
still don't get some of the cases.

Can you please explain when you are once more awake..

                Linus