linux-kernel - Re: kernel BUG at fs/pipe.c:LINE!

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] [day] [month] [year] [list]

Message-ID: <27081.1575551190@warthog.procyon.org.uk>
Date:   Thu, 05 Dec 2019 13:06:30 +0000
From:   David Howells <dhowells@...hat.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     dhowells@...hat.com, amit@...nel.org, arnd@...db.de,
        syzbot <syzbot+d37abaade33a934f16f2@...kaller.appspotmail.com>,
        gregkh@...uxfoundation.org, jannh@...gle.com,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        miklos@...redi.hu, rostedt@...dmis.org,
        syzkaller-bugs@...glegroups.com, viro@...iv.linux.org.uk,
        virtualization@...ts.linux-foundation.org, willy@...radead.org
Subject: Re: kernel BUG at fs/pipe.c:LINE!

Eric Biggers <ebiggers@...nel.org> wrote:

> static __poll_t
> pipe_poll(struct file *filp, poll_table *wait)
> {
>         __poll_t mask;
>         struct pipe_inode_info *pipe = filp->private_data;
>         unsigned int head = READ_ONCE(pipe->head);
>         unsigned int tail = READ_ONCE(pipe->tail);
> 
>         poll_wait(filp, &pipe->wait, wait);
> 
>         BUG_ON(pipe_occupancy(head, tail) > pipe->ring_size);
> 
> It's not holding the pipe mutex, right?  So 'head', 'tail' and 'ring_size' can
> all be changed concurrently, and they aren't read atomically with respect to
> each other.
> 
> How do you propose to implement poll() correctly with the new head + tail
> approach?  Just take the mutex?

Firstly, the BUG_ON() check probably isn't necessary here - the same issue
with occupancy being seen to be greater than the queue depth existed
previously (there was no locking around the read of pipe->nrbufs and
pipe->buffers).  I added a sanity check.

Secondly, it should be possible to make it such that just the spinlock
suffices.  The following few patches make the main pipe read/write routines
use the spinlock so as not to be interfered with by notification insertion.

I didn't roll the spinlock out to splice and suchlike since I prohibit
splicing to a notifications pipe because of the iov_iter_revert() fun.

David