linux-kernel - Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAB0TPYEd-UeEeZbn7iDLwXYLRZfS5vSgY4hJTooYdCs3n6FTqA@mail.gmail.com>
Date:   Tue, 28 Jul 2020 18:01:54 +0200
From:   Martijn Coenen <maco@...roid.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <christian@...uner.io>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        Mattias Nissler <mnissler@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0

Thanks a lot for the detailed explanation, I understood now.

Martijn

On Tue, Jul 28, 2020 at 4:50 PM Jann Horn <jannh@...gle.com> wrote:
>
> On Tue, Jul 28, 2020 at 3:50 PM Martijn Coenen <maco@...roid.com> wrote:
> > On Mon, Jul 27, 2020 at 2:04 PM Jann Horn <jannh@...gle.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.