linux-kernel - Re: [PATCH 2/2] aio: fix use-after-free due to missing POLLFREE handling

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]

Message-ID: <Ya5qWLLv3i4szS4N@gmail.com>
Date:   Mon, 6 Dec 2021 11:54:00 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Benjamin LaHaise <bcrl@...ck.org>, linux-aio@...ck.org,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ramji Jiyani <ramjiyani@...gle.com>,
        Christoph Hellwig <hch@....de>,
        Oleg Nesterov <oleg@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH 2/2] aio: fix use-after-free due to missing POLLFREE
 handling

On Mon, Dec 06, 2021 at 11:28:13AM -0800, Linus Torvalds wrote:
> On Fri, Dec 3, 2021 at 4:23 PM Eric Biggers <ebiggers@...nel.org> wrote:
> >
> > require another solution.  This solution is for the queue to be cleared
> > before it is freed, using 'wake_up_poll(wq, EPOLLHUP | POLLFREE);'.
> 
> Ugh.
> 
> I hate POLLFREE, and the more I look at this, the more I think it's broken.
> 
> And that
> 
>         wake_up_poll(wq, EPOLLHUP | POLLFREE);
> 
> in particular looks broken - the intent is that it should remove all
> the wait queue entries (because the wait queue head is going away),
> but wake_up_poll() iself actually does
> 
>         __wake_up(x, TASK_NORMAL, 1, poll_to_key(m))
> 
> where that '1' is the number of exclusive entries it will wake up.
> 
> So if there are two exclusive waiters, wake_up_poll() will simply stop
> waking things up after the first one.
> 
> Which defeats the whole POLLFREE thing too.
> 
> Maybe I'm missing something, but POLLFREE really is broken.
> 
> I'd argue that all of epoll() is broken, but I guess we're stuck with it.
> 
> Now, it's very possible that nobody actually uses exclusive waits for
> those wait queues, and my "nr_exclusive" argument is about something
> that isn't actually a bug in reality. But I think it's a sign of
> confusion, and it's just another issue with POLLFREE.
> 
> I really wish we could have some way to not have epoll and aio mess
> with the wait-queue lists and cache the wait queue head pointers that
> they don't own.
> 
> In the meantime, I don't think these patches make things worse, and
> they may fix things. But see above about "nr_exclusive" and how I
> think wait queue entries might end up avoiding POLLFREE handling..
> 
>                   Linus

epoll supports exclusive waits, via the EPOLLEXCLUSIVE flag.  So this looks like
a real problem.

It could be fixed by converting signalfd and binder to use something like this,
right?

	#define wake_up_pollfree(x)  \
	       __wake_up(x, TASK_NORMAL, 0, poll_to_key(EPOLLHUP | POLLFREE))

As for eliminating POLLFREE entirely, that would require that the waitqueue
heads be moved to a location which has a longer lifetime.  I'm not sure if
that's possible.  In the case of signalfd, maybe the waitqueue head could be
moved to the file private data (signalfd_ctx), and then sighand_struct would
contain a list of signalfd_ctx's which are receiving signals directed to that
sighand_struct, rather than the waitqueue head itself.  I'm not sure how well
that would work.  This would probably change user-visible behavior; if a
signalfd is inherited by fork(), the child process would be notified about
signals sent to the parent process, rather than itself as is currently the case.

- Eric