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]

Date:   Mon, 7 Oct 2019 11:31:46 +0200
From:   Martijn Coenen <maco@...roid.com>
To:     Mattias Nissler <mnissler@...omium.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Todd Kjos <tkjos@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.

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