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: [email protected]
Signed-off-by: Hridya Valsaraju <[email protected]>
---
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;
--
2.22.0.510.g264f2c817a-goog
On Mon, Jul 15, 2019 at 12:18 PM Hridya Valsaraju <[email protected]> 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: [email protected]
> Signed-off-by: Hridya Valsaraju <[email protected]>
Acked-by: Todd Kjos <[email protected]>
> ---
> 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;
> --
> 2.22.0.510.g264f2c817a-goog
>
On Mon, Jul 15, 2019 at 9:18 PM Hridya Valsaraju <[email protected]> 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: [email protected]
> Signed-off-by: Hridya Valsaraju <[email protected]>
> ---
> 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/[email protected]/>
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);
}
=================
On Fri, Oct 11, 2019 at 11:59 PM Jann Horn <[email protected]> wrote:
> (I think you could also let A receive a handle
> to itself and then transact with itself, but I haven't tested that.)
Ignore this sentence, that's obviously wrong because same-binder_proc
nodes will always show up as a binder, not a handle.
On Fri, Oct 11, 2019 at 3:11 PM Jann Horn <[email protected]> wrote:
>
> On Fri, Oct 11, 2019 at 11:59 PM Jann Horn <[email protected]> wrote:
> > (I think you could also let A receive a handle
> > to itself and then transact with itself, but I haven't tested that.)
>
> Ignore this sentence, that's obviously wrong because same-binder_proc
> nodes will always show up as a binder, not a handle.
Thank you for the email and steps to reproduce the issue Jann. I need
some time to take a look at the same and I will get back to you once I
understand it and hopefully have a fix. We do want to disallow
same-process transactions. Here is a little bit more of context for
the patch: https://lkml.org/lkml/2018/3/28/173
On Mon, Oct 14, 2019 at 7:38 PM Hridya Valsaraju <[email protected]> wrote:
> On Fri, Oct 11, 2019 at 3:11 PM Jann Horn <[email protected]> wrote:
> > On Fri, Oct 11, 2019 at 11:59 PM Jann Horn <[email protected]> wrote:
> > > (I think you could also let A receive a handle
> > > to itself and then transact with itself, but I haven't tested that.)
> >
> > Ignore this sentence, that's obviously wrong because same-binder_proc
> > nodes will always show up as a binder, not a handle.
>
> Thank you for the email and steps to reproduce the issue Jann. I need
> some time to take a look at the same and I will get back to you once I
> understand it and hopefully have a fix. We do want to disallow
> same-process transactions. Here is a little bit more of context for
> the patch: https://lkml.org/lkml/2018/3/28/173
That patch (commit 7aa135fcf26377f92dc0680a57566b4c7f3e281b) prevented
transactions within one *binder_proc*, which makes sense to me; that
still allows same-process transactions, so long as they are between
different binder_proc instances. What I don't understand is your
follow-up in commit 49ed96943a8e0c62cc5a9b0a6cfc88be87d1fcec, where
you try to block transactions within the same process (well, kind of,
the semantics of the term "process" are quite fuzzy here and don't map
onto binder well).