Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Oct 07, 2019 at 11:33:53AM +0200, Martijn Coenen wrote:
> On Sun, Oct 6, 2019 at 7:23 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > From: Martijn Coenen <maco@android.com>
> >
> > commit f5cb779ba16334b45ba8946d6bfa6d9834d1527f upstream.
> >
> > binder_poll() passes the thread->wait waitqueue that
> > can be slept on for work. When a thread that uses
> > epoll explicitly exits using BINDER_THREAD_EXIT,
> > the waitqueue is freed, but it is never removed
> > from the corresponding epoll data structure. When
> > the process subsequently exits, the epoll cleanup
> > code tries to access the waitlist, which results in
> > a use-after-free.
> >
> > Prevent this by using POLLFREE when the thread exits.
> >
> > Signed-off-by: Martijn Coenen <maco@android.com>
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Cc: stable <stable@vger.kernel.org> # 4.14
> > [backport BINDER_LOOPER_STATE_POLL logic as well]
> > Signed-off-by: Mattias Nissler <mnissler@chromium.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/android/binder.c |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -334,7 +334,8 @@ enum {
> >         BINDER_LOOPER_STATE_EXITED      = 0x04,
> >         BINDER_LOOPER_STATE_INVALID     = 0x08,
> >         BINDER_LOOPER_STATE_WAITING     = 0x10,
> > -       BINDER_LOOPER_STATE_NEED_RETURN = 0x20
> > +       BINDER_LOOPER_STATE_NEED_RETURN = 0x20,
> > +       BINDER_LOOPER_STATE_POLL        = 0x40,
> >  };
> >
> >  struct binder_thread {
> > @@ -2628,6 +2629,18 @@ static int binder_free_thread(struct bin
> >                 } else
> >                         BUG();
> >         }
> > +
> > +       /*
> > +        * If this thread used poll, make sure we remove the waitqueue
> > +        * from any epoll data structures holding it with POLLFREE.
> > +        * waitqueue_active() is safe to use here because we're holding
> > +        * the inner lock.
> 
> This should be "global lock" in 4.9 and 4.4 :)

I'll go update the comment now, thanks!

> Otherwise LGTM, thanks!

thanks for the review.

greg k-h