Date:   Fri, 11 Oct 2019 23:59:42 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Hridya Valsaraju <hridya@...gle.com>, Todd Kjos <tkjos@...roid.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <christian@...uner.io>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        kernel-team <kernel-team@...roid.com>,
        syzbot+8b3c354d33c4ac78bfad@...kaller.appspotmail.com
Subject: Re: [PATCH] binder: prevent transactions to context manager from its
 own process.

On Mon, Jul 15, 2019 at 9:18 PM Hridya Valsaraju <hridya@...gle.com> wrote:
> Currently, a transaction to context manager from its own process
> is prevented by checking if its binder_proc struct is the same as
> that of the sender. However, this would not catch cases where the
> process opens the binder device again and uses the new fd to send
> a transaction to the context manager.
>
> Reported-by: syzbot+8b3c354d33c4ac78bfad@...kaller.appspotmail.com
> Signed-off-by: Hridya Valsaraju <hridya@...gle.com>
> ---
>  drivers/android/binder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index e4d25ebec5be..89b9cedae088 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3138,7 +3138,7 @@ static void binder_transaction(struct binder_proc *proc,
>                         else
>                                 return_error = BR_DEAD_REPLY;
>                         mutex_unlock(&context->context_mgr_node_lock);
> -                       if (target_node && target_proc == proc) {
> +                       if (target_node && target_proc->pid == proc->pid) {
>                                 binder_user_error("%d:%d got transaction to context manager from process owning it\n",
>                                                   proc->pid, thread->pid);
>                                 return_error = BR_FAILED_REPLY;

This isn't a valid fix.

For context, the syzkaller report at
<https://lore.kernel.org/lkml/000000000000afe2c70589526668@google.com/>
triggered this WARN_ON() in binder_transaction_buffer_release() in the
BINDER_TYPE_FD case, which Todd added in 44d8047f1d87 ("binder: use
standard functions to allocate fds"):

    case BINDER_TYPE_FD: {
        /*
         * No need to close the file here since user-space
         * closes it for for successfully delivered
         * transactions. For transactions that weren't
         * delivered, the new fd was never allocated so
         * there is no need to close and the fput on the
         * file is done when the transaction is torn
         * down.
         */
        WARN_ON(failed_at &&
            proc->tsk == current->group_leader);
    } break;

That check seems to be attempting to detect cases where
binder_transaction() fails and rolls back a partial transaction sent
by a process to itself. I think the intent there is probably to catch
cases that would cause the check in the BINDER_TYPE_FDA case below to
trip up?

About this fix: This prevents a task from sending binder transactions
to the context manager if they're running in the same process. (By the
way, I don't understand why that's a problem, conceptually.) But you
can still open a binder device twice (binder_proc instances A and B)
from a process that does not own the context manager instance, pass a
binder object from A to the context manager, let the context manager
pass it to B, and then A can transact with the same-process B. So this
merely looks fixed because syzkaller isn't able to construct such a
complicated testcase. (I think you could also let A receive a handle
to itself and then transact with itself, but I haven't tested that.)


I think this fix should probably be reverted (unless you actually want
to prevent intra-process transactions, which would probably require a
bunch of ugly extra checks), the WARN_ON() should be removed, and the
BINDER_TYPE_FDA case should be adjusted to make its decision based on
a flag passed from its parent instead of guessing based on what
`current` is. Since it looks like because of this bug, an aborted
intra-process transaction containing BINDER_TYPE_FDA (e.g. via the
err_translate_failed or err_dead_proc_or_thread cases) will cause file
descriptors to unexpectedly be released in the caller, leading to a
file-descriptor use-after-free in userspace, the fix should probably
also be stable-backported. (It's probably not a huge problem in
practice though, given that only hwbinder uses BINDER_TYPE_FDA and you
need to have an intra-process transaction at the same time as
something like a thread going away, or something like that? I don't
fully understand the failure conditions for binder transactions.)


Here's a reproducer for triggering the WARN_ON() on git master. The
helper files binder.c and binder.h are attached.

=================
#define _GNU_SOURCE
#include <unistd.h>
#include <stdio.h>
#include <stdint.h>
#include <err.h>
#include <stdlib.h>
#include <sys/signal.h>
#include <sys/prctl.h>
#include "binder.h"

#define BINDER_PATH "/dev/binder/binder"

static void do_exit(int dummy) {
  _exit(1);
}

static uint32_t ref_a_from_manager;

int my_handler(struct binder_state *bs, struct binder_transaction_data *txn,
               struct binder_io *msg, struct binder_io *reply) {
  if (txn->code == 1) {
    ref_a_from_manager = bio_get_ref(msg);
    if (ref_a_from_manager == 0)
      errx(1, "manager received bogus message 1");
    binder_acquire(bs, ref_a_from_manager);
    printf("manager received handle 0x%x from A\n", ref_a_from_manager);
    return 0;
  } else if (txn->code == 2) {
    if (ref_a_from_manager == 0)
      errx(1, "B asked too early");
    bio_put_ref(reply, ref_a_from_manager);
    printf("manager is sending handle to B\n");
    return 0;
  } else {
    errx(1, "manager got unexpected message");
  }
}

int main(void) {
  if (signal(SIGCHLD, do_exit))
    err(1, "signal");

  struct binder_state *bs_mgr = binder_open(BINDER_PATH, 0x400000);
  if (bs_mgr == NULL)
    err(1, "binder_open()");
  if (binder_become_context_manager(bs_mgr))
    err(1, "become mgr");

  pid_t child = fork();
  if (child == -1)
    err(1, "fork");
  if (child == 0) {
    prctl(PR_SET_PDEATHSIG, SIGKILL);
    if (getppid() == 1) exit(0);

    /* create endpoint A and send message with handle to manager */
    {
      struct binder_state *bs_a = binder_open(BINDER_PATH, 0x400000);
      if (bs_a == NULL) err(1, "binder_open()");

        struct binder_io msg;
        struct binder_io reply;
        char data[0x1000];
        bio_init(&msg, data, sizeof(data), 4);
        bio_put_obj(&msg, (void*)1);
        if (binder_call(bs_a, &msg, &reply, 0, 1/*code*/))
          errx(1, "binder_call");
        binder_done(bs_a, &msg, &reply);
    }

    /* create endpoint B and retrieve handle from manager */
    struct binder_state *bs_b;
    uint32_t ref_a_from_b;
    {
      bs_b = binder_open(BINDER_PATH, 0x400000);
      if (bs_b == NULL) err(1, "binder_open()");

        struct binder_io msg;
        struct binder_io reply;
        char data[0x1000];
        bio_init(&msg, data, sizeof(data), 4);
        if (binder_call(bs_b, &msg, &reply, 0, 2/*code*/))
          errx(1, "binder_call");
        ref_a_from_b = bio_get_ref(&reply);
        if (ref_a_from_b == 0)
          errx(1, "B received bogus reply");
        binder_acquire(bs_b, ref_a_from_b);
        printf("B received handle 0x%x from manager\n", ref_a_from_b);
        binder_done(bs_b, &msg, &reply);
    }

    /* let B send a message with a valid FD and an invalid FD to A */
    {
      struct binder_io msg;
      struct binder_io reply;
      char data[0x1000];
      bio_init(&msg, data, sizeof(data), 4);
      bio_put_fd(&msg, 0); /*valid*/
      bio_put_fd(&msg, -1); /*invalid*/
      if (binder_call(bs_b, &msg, &reply, ref_a_from_b, 3/*code*/))
        errx(1, "binder_call");
    }

    exit(0);
  }

  binder_loop(bs_mgr, my_handler);
}
=================

View attachment "binder.h" of type "text/x-chdr" (4181 bytes)

View attachment "binder.c" of type "text/x-csrc" (25964 bytes)

Powered by blists - more mailing lists