linux-kernel - Re: [PATCH 1/6] seq_file: add seq_read

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:   Wed, 11 Nov 2020 21:52:20 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christoph Hellwig <hch@....de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/6] seq_file: add seq_read_iter

On Wed, Nov 11, 2020 at 09:54:12AM -0800, Linus Torvalds wrote:
> On Tue, Nov 10, 2020 at 3:20 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > Any objections to the following?
> 
> Well, I don't _object_, but I find it ugly.
> 
> And I think both the old and the "fixed" code is wrong when an EFAULT
> happens in the middle.
> 
> Yes, we can just return EFAULT. But for read() and write() we really
> try to do the proper partial returns in other places, why not here?
> 
> IOW, why isn't the proper fix just something like this:
> 
>     diff --git a/fs/seq_file.c b/fs/seq_file.c
>     index 3b20e21604e7..ecc6909b71f5 100644
>     --- a/fs/seq_file.c
>     +++ b/fs/seq_file.c
>     @@ -209,7 +209,8 @@ ssize_t seq_read_iter(struct kiocb *iocb,
> struct iov_iter *iter)
>         /* if not empty - flush it first */
>         if (m->count) {
>                 n = min(m->count, size);
>     -           if (copy_to_iter(m->buf + m->from, n, iter) != n)
>     +           n = copy_to_iter(m->buf + m->from, n, iter);
>     +           if (!n)
>                         goto Efault;
>                 m->count -= n;
>                 m->from += n;
> 
> which should get the "efault in the middle" case roughly right (ie the
> usual "exact byte alignment and page crosser" caveats apply).

Look at the loop after that one, specifically the "it doesn't fit,
allocate a bigger one" part:
                kvfree(m->buf);
                m->count = 0;
                m->buf = seq_buf_alloc(m->size <<= 1);
It really depends upon having m->buf empty at the beginning of
the loop.  Your variant will lose the data, unless we copy the
"old" part of buffer (from before the ->show()) into the
larger one.

That can be done, but I would rather go with
		n = copy_to_iter(m->buf + m->from, m->count, iter);
		m->count -= n;
		m->from += n;
                copied += n;
                if (!size)
                        goto Done;
		if (m->count)
			goto Efault;
if we do it that way.  Let me see if I can cook something
reasonable along those lines...