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

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: <20191007093819.GA84909@kroah.com>
Date:   Mon, 7 Oct 2019 11:38:19 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Martijn Coenen <maco@...roid.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org,
        syzbot <syzkaller@...glegroups.com>,
        Mattias Nissler <mnissler@...omium.org>
Subject: Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread
 exits.

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@...uxfoundation.org> wrote:
> >
> > From: Martijn Coenen <maco@...roid.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@...roid.com>
> > Reported-by: syzbot <syzkaller@...glegroups.com>
> > Cc: stable <stable@...r.kernel.org> # 4.14
> > [backport BINDER_LOOPER_STATE_POLL logic as well]
> > Signed-off-by: Mattias Nissler <mnissler@...omium.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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