From: Martijn Coenen <maco@android.com>
To: Jann Horn <jannh@google.com>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Arve Hjønnevåg" <arve@android.com>,
"Todd Kjos" <tkjos@android.com>,
"Joel Fernandes" <joel@joelfernandes.org>,
"Christian Brauner" <christian@brauner.io>,
"open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
"Mattias Nissler" <mnissler@google.com>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
Date: Tue, 28 Jul 2020 15:50:30 +0200 [thread overview]
Message-ID: <CAB0TPYHx-SkaWR6i0LwLYRJv13_0V3Ak63H0=parZNvS5XdqgA@mail.gmail.com> (raw)
In-Reply-To: <20200727120424.1627555-1-jannh@google.com>
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
>
next prev parent reply other threads:[~2020-07-28 13:50 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-27 12:04 [PATCH v2] binder: Prevent context manager from incrementing ref 0 Jann Horn
2020-07-28 13:50 ` Martijn Coenen [this message]
2020-07-28 14:49 ` Jann Horn
2020-07-28 16:01 ` Martijn Coenen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAB0TPYHx-SkaWR6i0LwLYRJv13_0V3Ak63H0=parZNvS5XdqgA@mail.gmail.com' \
--to=maco@android.com \
--cc=arve@android.com \
--cc=christian@brauner.io \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=jannh@google.com \
--cc=joel@joelfernandes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mnissler@google.com \
--cc=tkjos@android.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).