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