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

[Martijn Coenen]

On Mon, Oct 7, 2019 at 8:28 AM Mattias Nissler <mnissler@xxxxxxxxxxxx> wrote:
> 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.

Yes, I think this is correct; either the binder fd is closed first, in
which case eventpoll_release() removes the waitqueue from the list
before it is freed (before binder's release() is called); instead if
the epoll fd is closed first, it will likewise remove the waitqueue
itself, before binder_proc can be freed.. I don't know the __fput()
code that well, but at first glance it seems these two can't overlap.

The whole problem with BINDER_THREAD_EXIT was that the returned
waitqueue wasn't tied to the lifetime of the underlying file.

Apologies for not spotting this needed a backport BTW - I refactored
the wait code heavily somewhere between 4.9 and 4.14, and somehow
didn't realize the same problem existed in the old code.

Thanks,
Martijn

>
> > >
> > >         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