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;
MIME-Version: 1.0
References: <20191006172016.873463083@linuxfoundation.org> <20191006172018.480360174@linuxfoundation.org>
 <20191006173202.GA832@sol.localdomain> <20191006182433.GA217738@kroah.com> <CAKUbbxKbR6R_VM233pkw7_rxv_DMJoBPC_U5ZgVkR6GpXVAScw@mail.gmail.com>
In-Reply-To: <CAKUbbxKbR6R_VM233pkw7_rxv_DMJoBPC_U5ZgVkR6GpXVAScw@mail.gmail.com>
From:   Martijn Coenen <maco@android.com>
Date:   Mon, 7 Oct 2019 11:31:46 +0200
Message-ID: <CAB0TPYGjhzqAK5pAP3rjgqq9=9LA4z2Xpv+QG8sDrEkd+HWDyA@mail.gmail.com>
Subject: Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
To:     Mattias Nissler <mnissler@chromium.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Todd Kjos <tkjos@google.com>,
        LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org,
        syzbot <syzkaller@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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