(resend, apologies for accidental HTML reply)
On Sun, Oct 6, 2019 at 11:24 AM Greg Kroah-Hartman gregkh@linuxfoundation.org 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@android.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@android.com Reported-by: syzbot syzkaller@googlegroups.com Cc: stable stable@vger.kernel.org # 4.14 [backport BINDER_LOOPER_STATE_POLL logic as well] Signed-off-by: Mattias Nissler mnissler@chromium.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.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.
- */
- 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