Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Oct 2019 11:38:19 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Martijn Coenen <maco@android.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org,
        syzbot <syzkaller@googlegroups.com>,
        Mattias Nissler <mnissler@chromium.org>
Subject: Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread
 exits.
Message-ID: <20191007093819.GA84909@kroah.com>
References: <20191006172016.873463083@linuxfoundation.org>
 <20191006172018.480360174@linuxfoundation.org>
 <CAB0TPYGO8Nm_Qz0kzSvX69NApiPwu4xV19F=KhyLe5DO3DoLTw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAB0TPYGO8Nm_Qz0kzSvX69NApiPwu4xV19F=KhyLe5DO3DoLTw@mail.gmail.com>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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