On Mon, Jul 15, 2019 at 9:18 PM Hridya Valsaraju 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@syzkaller.appspotmail.com > Signed-off-by: Hridya Valsaraju > --- > 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 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 #include #include #include #include #include #include #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); } =================