Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
On Mon, Oct 07, 2019 at 11:33:53AM +0200, Martijn Coenen wrote: > On Sun, Oct 6, 2019 at 7:23 PM Greg Kroah-Hartman > wrote: > > > > From: Martijn Coenen > > > > 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 > > Reported-by: syzbot > > Cc: stable # 4.14 > > [backport BINDER_LOOPER_STATE_POLL logic as well] > > Signed-off-by: Mattias Nissler > > Signed-off-by: Greg Kroah-Hartman > > --- > > 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. > > This should be "global lock" in 4.9 and 4.4 :) I'll go update the comment now, thanks! > Otherwise LGTM, thanks! thanks for the review. greg k-h
Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
On Sun, Oct 6, 2019 at 7:23 PM Greg Kroah-Hartman wrote: > > From: Martijn Coenen > > 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 > Reported-by: syzbot > Cc: stable # 4.14 > [backport BINDER_LOOPER_STATE_POLL logic as well] > Signed-off-by: Mattias Nissler > Signed-off-by: Greg Kroah-Hartman > --- > 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. This should be "global lock" in 4.9 and 4.4 :) Otherwise LGTM, thanks! Martijn > +*/ > + 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; > > >
Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
On Mon, Oct 7, 2019 at 8:28 AM Mattias Nissler 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
Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
(resend, apologies for accidental HTML reply) On Sun, Oct 6, 2019 at 11:24 AM Greg Kroah-Hartman 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 > > > > > > 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 > > > Reported-by: syzbot > > > Cc: stable # 4.14 > > > [backport BINDER_LOOPER_STATE_POLL logic as well] > > > Signed-off-by: Mattias Nissler > > > Signed-off-by: Greg Kroah-Hartman > > > --- > > > 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
Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
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 > > > > 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 > > Reported-by: syzbot > > Cc: stable # 4.14 > > [backport BINDER_LOOPER_STATE_POLL logic as well] > > Signed-off-by: Mattias Nissler > > Signed-off-by: Greg Kroah-Hartman > > --- > > 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?: > > 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
Re: [PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
On Sun, Oct 06, 2019 at 07:21:17PM +0200, Greg Kroah-Hartman wrote: > From: Martijn Coenen > > 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 > Reported-by: syzbot > Cc: stable # 4.14 > [backport BINDER_LOOPER_STATE_POLL logic as well] > Signed-off-by: Mattias Nissler > Signed-off-by: Greg Kroah-Hartman > --- > 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?: 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;
[PATCH 4.9 30/47] ANDROID: binder: remove waitqueue when thread exits.
From: Martijn Coenen 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 Reported-by: syzbot Cc: stable # 4.14 [backport BINDER_LOOPER_STATE_POLL logic as well] Signed-off-by: Mattias Nissler Signed-off-by: Greg Kroah-Hartman --- 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;