Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200727120424.1627555-1-jannh@google.com> <CAB0TPYHx-SkaWR6i0LwLYRJv13_0V3Ak63H0=parZNvS5XdqgA@mail.gmail.com>
In-Reply-To: <CAB0TPYHx-SkaWR6i0LwLYRJv13_0V3Ak63H0=parZNvS5XdqgA@mail.gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Tue, 28 Jul 2020 16:49:58 +0200
Message-ID: <CAG48ez23-oOGjeNhFCzrWxJuuJUsdgbpiSiPvM0ov1n+FcLPhw@mail.gmail.com>
Subject: Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0
To:     Martijn Coenen <maco@android.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.