DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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, back to index

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

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git