* [PATCH v2] binder: Prevent context manager from incrementing ref 0
@ 2020-07-27 12:04 Jann Horn
2020-07-28 13:50 ` Martijn Coenen
2020-07-30 13:48 ` Sasha Levin
0 siblings, 2 replies; 5+ messages in thread
From: Jann Horn @ 2020-07-27 12:04 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner
Cc: devel, Mattias Nissler, linux-kernel
Binder is designed such that a binder_proc never has references to
itself. If this rule is violated, memory corruption can occur when a
process sends a transaction to itself; see e.g.
<https://syzkaller.appspot.com/bug?extid=09e05aba06723a94d43d>.
There is a remaining edgecase through which such a transaction-to-self
can still occur from the context of a task with BINDER_SET_CONTEXT_MGR
access:
- task A opens /dev/binder twice, creating binder_proc instances P1
and P2
- P1 becomes context manager
- P2 calls ACQUIRE on the magic handle 0, allocating index 0 in its
handle table
- P1 dies (by closing the /dev/binder fd and waiting a bit)
- P2 becomes context manager
- P2 calls ACQUIRE on the magic handle 0, allocating index 1 in its
handle table
[this triggers a warning: "binder: 1974:1974 tried to acquire
reference to desc 0, got 1 instead"]
- task B opens /dev/binder once, creating binder_proc instance P3
- P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
transaction)
- P2 receives the handle and uses it to call P3 (two-way transaction)
- P3 calls P2 (via magic handle 0) (two-way transaction)
- P2 calls P2 (via handle 1) (two-way transaction)
And then, if P2 does *NOT* accept the incoming transaction work, but
instead closes the binder fd, we get a crash.
Solve it by preventing the context manager from using ACQUIRE on ref 0.
There shouldn't be any legitimate reason for the context manager to do
that.
Additionally, print a warning if someone manages to find another way to
trigger a transaction-to-self bug in the future.
Cc: stable@vger.kernel.org
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Acked-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Jann Horn <jannh@google.com>
---
fixed that broken binder_user_error() from the first version...
I sent v1 while I had a dirty tree containing the missing fix. whoops.
drivers/android/binder.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f50c5f182bb5..5b310eea9e52 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2982,6 +2982,12 @@ static void binder_transaction(struct binder_proc *proc,
goto err_dead_binder;
}
e->to_node = target_node->debug_id;
+ if (WARN_ON(proc == target_proc)) {
+ return_error = BR_FAILED_REPLY;
+ return_error_param = -EINVAL;
+ return_error_line = __LINE__;
+ goto err_invalid_target_handle;
+ }
if (security_binder_transaction(proc->tsk,
target_proc->tsk) < 0) {
return_error = BR_FAILED_REPLY;
@@ -3635,10 +3641,17 @@ static int binder_thread_write(struct binder_proc *proc,
struct binder_node *ctx_mgr_node;
mutex_lock(&context->context_mgr_node_lock);
ctx_mgr_node = context->binder_context_mgr_node;
- if (ctx_mgr_node)
+ if (ctx_mgr_node) {
+ if (ctx_mgr_node->proc == proc) {
+ binder_user_error("%d:%d context manager tried to acquire desc 0\n",
+ proc->pid, thread->pid);
+ mutex_unlock(&context->context_mgr_node_lock);
+ return -EINVAL;
+ }
ret = binder_inc_ref_for_node(
proc, ctx_mgr_node,
strong, NULL, &rdata);
+ }
mutex_unlock(&context->context_mgr_node_lock);
}
if (ret)
base-commit: 2a89b99f580371b86ae9bafd6cbeccd3bfab524a
--
2.28.0.rc0.142.g3c755180ce-goog
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
2020-07-27 12:04 [PATCH v2] binder: Prevent context manager from incrementing ref 0 Jann Horn
@ 2020-07-28 13:50 ` Martijn Coenen
2020-07-28 14:49 ` Jann Horn
2020-07-30 13:48 ` Sasha Levin
1 sibling, 1 reply; 5+ messages in thread
From: Martijn Coenen @ 2020-07-28 13:50 UTC (permalink / raw)
To: Jann Horn
Cc: open list:ANDROID DRIVERS, Todd Kjos, Greg Kroah-Hartman, LKML,
Arve Hjønnevåg, Joel Fernandes, Mattias Nissler,
Christian Brauner
Thanks Jann, the change LGTM, one question on the repro scenario that
wasn't immediately obvious to me:
On Mon, Jul 27, 2020 at 2:04 PM Jann Horn <jannh@google.com> wrote:
> - task B opens /dev/binder once, creating binder_proc instance P3
> - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
> transaction)
> - P2 receives the handle and uses it to call P3 (two-way transaction)
> - P3 calls P2 (via magic handle 0) (two-way transaction)
> - P2 calls P2 (via handle 1) (two-way transaction)
Why do you need P3 involved at all? Could P2 just straight away make a
call on handle 1?
>
> And then, if P2 does *NOT* accept the incoming transaction work, but
> instead closes the binder fd, we get a crash.
>
> Solve it by preventing the context manager from using ACQUIRE on ref 0.
> There shouldn't be any legitimate reason for the context manager to do
> that.
>
> Additionally, print a warning if someone manages to find another way to
> trigger a transaction-to-self bug in the future.
>
> Cc: stable@vger.kernel.org
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Acked-by: Todd Kjos <tkjos@google.com>
Reviewed-by: Martijn Coenen <maco@android.com>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> fixed that broken binder_user_error() from the first version...
> I sent v1 while I had a dirty tree containing the missing fix. whoops.
>
> drivers/android/binder.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f50c5f182bb5..5b310eea9e52 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2982,6 +2982,12 @@ static void binder_transaction(struct binder_proc *proc,
> goto err_dead_binder;
> }
> e->to_node = target_node->debug_id;
> + if (WARN_ON(proc == target_proc)) {
> + return_error = BR_FAILED_REPLY;
> + return_error_param = -EINVAL;
> + return_error_line = __LINE__;
> + goto err_invalid_target_handle;
> + }
> if (security_binder_transaction(proc->tsk,
> target_proc->tsk) < 0) {
> return_error = BR_FAILED_REPLY;
> @@ -3635,10 +3641,17 @@ static int binder_thread_write(struct binder_proc *proc,
> struct binder_node *ctx_mgr_node;
> mutex_lock(&context->context_mgr_node_lock);
> ctx_mgr_node = context->binder_context_mgr_node;
> - if (ctx_mgr_node)
> + if (ctx_mgr_node) {
> + if (ctx_mgr_node->proc == proc) {
> + binder_user_error("%d:%d context manager tried to acquire desc 0\n",
> + proc->pid, thread->pid);
> + mutex_unlock(&context->context_mgr_node_lock);
> + return -EINVAL;
> + }
> ret = binder_inc_ref_for_node(
> proc, ctx_mgr_node,
> strong, NULL, &rdata);
> + }
> mutex_unlock(&context->context_mgr_node_lock);
> }
> if (ret)
>
> base-commit: 2a89b99f580371b86ae9bafd6cbeccd3bfab524a
> --
> 2.28.0.rc0.142.g3c755180ce-goog
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
2020-07-28 13:50 ` Martijn Coenen
@ 2020-07-28 14:49 ` Jann Horn
2020-07-28 16:01 ` Martijn Coenen
0 siblings, 1 reply; 5+ messages in thread
From: Jann Horn @ 2020-07-28 14:49 UTC (permalink / raw)
To: Martijn Coenen
Cc: open list:ANDROID DRIVERS, Todd Kjos, Greg Kroah-Hartman, LKML,
Arve Hjønnevåg, Joel Fernandes, Mattias Nissler,
Christian Brauner
On Tue, Jul 28, 2020 at 3:50 PM Martijn Coenen <maco@android.com> wrote:
> On Mon, Jul 27, 2020 at 2:04 PM Jann Horn <jannh@google.com> wrote:
> > - task B opens /dev/binder once, creating binder_proc instance P3
> > - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
> > transaction)
> > - P2 receives the handle and uses it to call P3 (two-way transaction)
> > - P3 calls P2 (via magic handle 0) (two-way transaction)
> > - P2 calls P2 (via handle 1) (two-way transaction)
>
> Why do you need P3 involved at all? Could P2 just straight away make a
> call on handle 1?
Yes, it could, I think. IIRC these steps are necessary if you want to
actually get a KASAN splat, instead of just a transaction-to-self with
no further consequences. It has been a while since I looked at this,
but I'll try to figure out again what was going on...
A UAF occurs in the following code due to the transaction-to-self,
because the "if (t->to_thread == thread)" is tricked into believing
that the transaction has already been accepted.
static int binder_thread_release(struct binder_proc *proc,
struct binder_thread *thread)
{
struct binder_transaction *t;
struct binder_transaction *send_reply = NULL;
[...]
t = thread->transaction_stack;
if (t) {
[...]
if (t->to_thread == thread)
send_reply = t;
} else {
[...]
}
[...]
//NOTE: transaction is freed here because top-of-stack is
// wrongly treated as already-accepted incoming transaction)
if (send_reply)
binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
//NOTE pending transaction work is processed here (transaction has not
// yet been accepted)
binder_release_work(proc, &thread->todo);
[...]
}
An unaccepted transaction will only have a non-NULL ->to_thread if the
transaction has a specific target thread; for a non-reply transaction,
that is only the case if it is a two-way transaction that was sent
while the binder call stack already contained the target task (iow,
the transaction looks like a synchronous callback invocation).
With the steps:
- P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
transaction)
- P2 receives the handle and uses it to call P3 (two-way transaction)
- P3 calls P2 (via magic handle 0) (two-way transaction)
- P2 calls P2 (via handle 1) (two-way transaction)
the call stack will look like this:
P3 -> P2 -> P3 -> P2 -> P2
The initial call from P3 to P2 was IIRC just to give P2 a handle to
P3; IIRC the relevant part of the call stack was:
P2 -> P3 -> P2 -> P2
Because P2 already occurs down in the call stack, the final
transaction "P2 -> P2" is considered to be a callback and is
thread-directed.
The reason why P3 has to be created from task B is the "if
(target_node && target_proc->pid == proc->pid)" check for transactions
to reference 0.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
2020-07-28 14:49 ` Jann Horn
@ 2020-07-28 16:01 ` Martijn Coenen
0 siblings, 0 replies; 5+ messages in thread
From: Martijn Coenen @ 2020-07-28 16:01 UTC (permalink / raw)
To: Jann Horn
Cc: open list:ANDROID DRIVERS, Todd Kjos, Greg Kroah-Hartman, LKML,
Arve Hjønnevåg, Joel Fernandes, Mattias Nissler,
Christian Brauner
Thanks a lot for the detailed explanation, I understood now.
Martijn
On Tue, Jul 28, 2020 at 4:50 PM Jann Horn <jannh@google.com> wrote:
>
> On Tue, Jul 28, 2020 at 3:50 PM Martijn Coenen <maco@android.com> wrote:
> > On Mon, Jul 27, 2020 at 2:04 PM Jann Horn <jannh@google.com> wrote:
> > > - task B opens /dev/binder once, creating binder_proc instance P3
> > > - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
> > > transaction)
> > > - P2 receives the handle and uses it to call P3 (two-way transaction)
> > > - P3 calls P2 (via magic handle 0) (two-way transaction)
> > > - P2 calls P2 (via handle 1) (two-way transaction)
> >
> > Why do you need P3 involved at all? Could P2 just straight away make a
> > call on handle 1?
>
> Yes, it could, I think. IIRC these steps are necessary if you want to
> actually get a KASAN splat, instead of just a transaction-to-self with
> no further consequences. It has been a while since I looked at this,
> but I'll try to figure out again what was going on...
>
>
> A UAF occurs in the following code due to the transaction-to-self,
> because the "if (t->to_thread == thread)" is tricked into believing
> that the transaction has already been accepted.
>
> static int binder_thread_release(struct binder_proc *proc,
> struct binder_thread *thread)
> {
> struct binder_transaction *t;
> struct binder_transaction *send_reply = NULL;
> [...]
> t = thread->transaction_stack;
> if (t) {
> [...]
> if (t->to_thread == thread)
> send_reply = t;
> } else {
> [...]
> }
> [...]
> //NOTE: transaction is freed here because top-of-stack is
> // wrongly treated as already-accepted incoming transaction)
> if (send_reply)
> binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> //NOTE pending transaction work is processed here (transaction has not
> // yet been accepted)
> binder_release_work(proc, &thread->todo);
> [...]
> }
>
> An unaccepted transaction will only have a non-NULL ->to_thread if the
> transaction has a specific target thread; for a non-reply transaction,
> that is only the case if it is a two-way transaction that was sent
> while the binder call stack already contained the target task (iow,
> the transaction looks like a synchronous callback invocation).
>
> With the steps:
>
> - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
> transaction)
> - P2 receives the handle and uses it to call P3 (two-way transaction)
> - P3 calls P2 (via magic handle 0) (two-way transaction)
> - P2 calls P2 (via handle 1) (two-way transaction)
>
> the call stack will look like this:
>
> P3 -> P2 -> P3 -> P2 -> P2
>
> The initial call from P3 to P2 was IIRC just to give P2 a handle to
> P3; IIRC the relevant part of the call stack was:
>
> P2 -> P3 -> P2 -> P2
>
> Because P2 already occurs down in the call stack, the final
> transaction "P2 -> P2" is considered to be a callback and is
> thread-directed.
>
>
> The reason why P3 has to be created from task B is the "if
> (target_node && target_proc->pid == proc->pid)" check for transactions
> to reference 0.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
2020-07-27 12:04 [PATCH v2] binder: Prevent context manager from incrementing ref 0 Jann Horn
2020-07-28 13:50 ` Martijn Coenen
@ 2020-07-30 13:48 ` Sasha Levin
1 sibling, 0 replies; 5+ messages in thread
From: Sasha Levin @ 2020-07-30 13:48 UTC (permalink / raw)
To: Sasha Levin, Jann Horn, Greg Kroah-Hartman; +Cc: devel, Mattias Nissler, stable
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 457b9a6f09f0 ("Staging: android: add binder driver").
The bot has tested the following trees: v5.7.11, v5.4.54, v4.19.135, v4.14.190, v4.9.231, v4.4.231.
v5.7.11: Build OK!
v5.4.54: Build OK!
v4.19.135: Build OK!
v4.14.190: Build OK!
v4.9.231: Failed to apply! Possible dependencies:
14db31814a9a ("binder: Deal with contexts in debugfs")
19c987241ca1 ("binder: separate out binder_alloc functions")
1b77e9dcc3da ("ANDROID: binder: remove proc waitqueue")
342e5c90b601 ("binder: Support multiple context managers")
408c68b17aea ("ANDROID: binder: push new transactions to waiting threads.")
4bfac80af3a6 ("binder: Add extra size to allocator")
512cf465ee01 ("binder: fix use-after-free in binder_transaction()")
7980240b6d63 ("binder: Add support for scatter-gather")
9630fe8839ba ("binder: introduce locking helper functions")
a056af42032e ("binder: Refactor binder_transact()")
ac4812c5ffbb ("binder: Support multiple /dev instances")
def95c73567d ("binder: Add support for file-descriptor arrays")
fdfb4a99b6ab ("binder: separate binder allocator structure from binder proc")
feba3900cabb ("binder: Split flat_binder_object")
v4.4.231: Failed to apply! Possible dependencies:
14db31814a9a ("binder: Deal with contexts in debugfs")
19c987241ca1 ("binder: separate out binder_alloc functions")
1b77e9dcc3da ("ANDROID: binder: remove proc waitqueue")
212265e5ad72 ("android: binder: More offset validation")
342e5c90b601 ("binder: Support multiple context managers")
408c68b17aea ("ANDROID: binder: push new transactions to waiting threads.")
4bfac80af3a6 ("binder: Add extra size to allocator")
512cf465ee01 ("binder: fix use-after-free in binder_transaction()")
7980240b6d63 ("binder: Add support for scatter-gather")
83050a4e2197 ("android: drivers: Avoid debugfs race in binder")
9630fe8839ba ("binder: introduce locking helper functions")
a056af42032e ("binder: Refactor binder_transact()")
a906d6931f3c ("android: binder: Sanity check at binder ioctl")
ac4812c5ffbb ("binder: Support multiple /dev instances")
def95c73567d ("binder: Add support for file-descriptor arrays")
feba3900cabb ("binder: Split flat_binder_object")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-30 13:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27 12:04 [PATCH v2] binder: Prevent context manager from incrementing ref 0 Jann Horn
2020-07-28 13:50 ` Martijn Coenen
2020-07-28 14:49 ` Jann Horn
2020-07-28 16:01 ` Martijn Coenen
2020-07-30 13:48 ` Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).