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

[Mattias Nissler]

(resend, apologies for accidental HTML reply)

On Sun, Oct 6, 2019 at 11:24 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, Oct 06, 2019 at 10:32:02AM -0700, Eric Biggers wrote:
> > On Sun, Oct 06, 2019 at 07:21:17PM +0200, Greg Kroah-Hartman wrote:
> > > From: Martijn Coenen <maco@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx>
> > > Cc: stable <stable@xxxxxxxxxxxxxxx> # 4.14
> > > [backport BINDER_LOOPER_STATE_POLL logic as well]
> > > Signed-off-by: Mattias Nissler <mnissler@xxxxxxxxxxxx>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  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.
> > > +    */
> > > +   if ((thread->looper & BINDER_LOOPER_STATE_POLL) &&
> > > +       waitqueue_active(&thread->wait)) {
> > > +           wake_up_poll(&thread->wait, POLLHUP | POLLFREE);
> > > +   }
> > > +
> > >     if (send_reply)
> > >             binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> > >     binder_release_work(&thread->todo);
> > > @@ -2651,6 +2664,8 @@ static unsigned int binder_poll(struct f
> > >             return POLLERR;
> > >     }
> > >
> > > +   thread->looper |= BINDER_LOOPER_STATE_POLL;
> > > +
> > >     wait_for_proc_work = thread->transaction_stack == NULL &&
> > >             list_empty(&thread->todo) && thread->return_error == BR_OK;
> > >
> >
> > Are you sure this backport is correct, given that in 4.9, binder_poll()
> > sometimes uses proc->wait instead of thread->wait?:

Jann's PoC calls the BINDER_THREAD_EXIT ioctl to free the
binder_thread which will then cause the UAF, and this is cut off by
the patch. IIUC, you are worried about a similar AUF on the proc->wait
access. I am not 100% sure, but I think the binder_proc lifetime
matches the corresponding struct file instance, so it shouldn't be
possible to get the binder_proc deallocated while still being able to
access it via filp->private_data.

> >
> >         wait_for_proc_work = thread->transaction_stack == NULL &&
> >                 list_empty(&thread->todo) && thread->return_error == BR_OK;
> >
> >         binder_unlock(__func__);
> >
> >         if (wait_for_proc_work) {
> >                 if (binder_has_proc_work(proc, thread))
> >                         return POLLIN;
> >                 poll_wait(filp, &proc->wait, wait);
> >                 if (binder_has_proc_work(proc, thread))
> >                         return POLLIN;
> >         } else {
> >                 if (binder_has_thread_work(thread))
> >                         return POLLIN;
> >                 poll_wait(filp, &thread->wait, wait);
> >                 if (binder_has_thread_work(thread))
> >                         return POLLIN;
> >         }
> >         return 0;
>
> I _think_ the backport is correct, and I know someone has verified that
> the 4.4.y backport works properly and I don't see much difference here
> from that version.
>
> But I will defer to Todd and Martijn here, as they know this code _WAY_
> better than I do.  The codebase has changed a lot from 4.9.y to 4.14.y
> so it makes it hard to do equal comparisons simply.
>
> Todd and Martijn, thoughts?
>
> thanks,
>
> greg k-h