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(>wait)) {
> + wake_up_poll(>wait, POLLHUP | POLLFREE);
> + }
> +
> if (send_reply)
> binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> binder_release_work(>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(>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(>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, >wait, wait);
> > > if (binder_has_proc_work(proc, thread))
> > > return POLLIN;
> > > } else {
> > > if (binder_has_thread_work(thread))
> > > return POLLIN;
> > > poll_wait(filp, >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(>wait)) {
> > > + wake_up_poll(>wait, POLLHUP | POLLFREE);
> > > + }
> > > +
> > > if (send_reply)
> > > binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> > > binder_release_work(>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(>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(>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, >wait, wait);
> > if (binder_has_proc_work(proc, thread))
> > return POLLIN;
> > } else {
> > if (binder_has_thread_work(thread))
> > return POLLIN;
> > poll_wait(filp, >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(>wait)) {
> > + wake_up_poll(>wait, POLLHUP | POLLFREE);
> > + }
> > +
> > if (send_reply)
> > binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> > binder_release_work(>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(>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(>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, >wait, wait);
> if (binder_has_proc_work(proc, thread))
> return POLLIN;
> } else {
> if (binder_has_thread_work(thread))
> return POLLIN;
> poll_wait(filp, >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(>wait)) {
> + wake_up_poll(>wait, POLLHUP | POLLFREE);
> + }
> +
> if (send_reply)
> binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> binder_release_work(>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(>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(>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, >wait, wait);
if (binder_has_proc_work(proc, thread))
return POLLIN;
} else {
if (binder_has_thread_work(thread))
return POLLIN;
poll_wait(filp, >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(>wait)) {
+ wake_up_poll(>wait, POLLHUP | POLLFREE);
+ }
+
if (send_reply)
binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
binder_release_work(>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(>todo) && thread->return_error == BR_OK;
