[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAG48ez0dSd4q06YXOnkzmM8BkfQGTtYE6j60_YRdC5fmrTm8jw@mail.gmail.com>
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