From: Martijn Coenen <maco@android.com>
To: Jann Horn <jannh@google.com>
Cc: "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
"Todd Kjos" <tkjos@android.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
LKML <linux-kernel@vger.kernel.org>,
"Arve Hjønnevåg" <arve@android.com>,
"Joel Fernandes" <joel@joelfernandes.org>,
"Mattias Nissler" <mnissler@google.com>,
"Christian Brauner" <christian@brauner.io>
Subject: Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
Date: Tue, 28 Jul 2020 18:01:54 +0200 [thread overview]
Message-ID: <CAB0TPYEd-UeEeZbn7iDLwXYLRZfS5vSgY4hJTooYdCs3n6FTqA@mail.gmail.com> (raw)
In-Reply-To: <CAG48ez23-oOGjeNhFCzrWxJuuJUsdgbpiSiPvM0ov1n+FcLPhw@mail.gmail.com>
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
next prev parent reply other threads:[~2020-07-28 16:02 UTC|newest]
Thread overview: 5+ 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
2020-07-28 14:49 ` Jann Horn
2020-07-28 16:01 ` Martijn Coenen [this message]
2020-07-30 13:48 ` Sasha Levin
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=CAB0TPYEd-UeEeZbn7iDLwXYLRZfS5vSgY4hJTooYdCs3n6FTqA@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).