binder: ensure binder_node has reference when starting transaction

When setting up a transaction, we look up a binder_ref based on
the given handle and use the associated node as the target_node
for the transaction. However, since we call binder_put_ref on
the reference after discovering the node, there is a risk that
the ref will be deleted -- and if that is the last ref on the
underlying node, the node could also be deleted. When we
call binder_inc_node(), it may enqueue work for the proc. There
is a timing window where this could result in a corrupt list
entry on the proc->todo list when the node is kfree'd.

Bug: 35933974
Change-Id: Ie351c9e080a33b0cf0354f357876877f9d59784d
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
This commit is contained in:
Todd Kjos
2017-03-03 14:22:51 -08:00
committed by Thierry Strudel
parent 8b8d920edd
commit f120798fab

View File

@@ -1762,6 +1762,7 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_proc *target_proc;
struct binder_thread *target_thread = NULL;
struct binder_node *target_node = NULL;
struct binder_ref *target_ref = NULL;
struct binder_worklist *target_list;
wait_queue_head_t *target_wait;
struct binder_transaction *in_reply_to = NULL;
@@ -1828,17 +1829,15 @@ static void binder_transaction(struct binder_proc *proc,
target_proc = target_thread->proc;
} else {
if (tr->target.handle) {
struct binder_ref *ref;
ref = binder_get_ref(proc, tr->target.handle, true);
if (ref == NULL) {
target_ref = binder_get_ref(proc, tr->target.handle,
true);
if (target_ref == NULL) {
binder_user_error("%d:%d got transaction to invalid handle\n",
proc->pid, thread->pid);
return_error = BR_FAILED_REPLY;
goto err_invalid_target_handle;
}
target_node = ref->node;
binder_put_ref(ref);
target_node = target_ref->node;
} else {
target_node = context->binder_context_mgr_node;
if (target_node == NULL) {
@@ -1954,8 +1953,12 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer->transaction = t;
t->buffer->target_node = target_node;
trace_binder_transaction_alloc_buf(t->buffer);
if (target_node)
if (target_node) {
binder_inc_node(target_node, 1, 0, NULL);
if (target_ref)
binder_put_ref(target_ref);
target_ref = NULL;
}
off_start = (binder_size_t *)(t->buffer->data +
ALIGN(tr->data_size, sizeof(void *)));
@@ -2196,6 +2199,9 @@ err_empty_call_stack:
err_dead_binder:
err_invalid_target_handle:
err_no_context_mgr_node:
if (target_ref)
binder_put_ref(target_ref);
binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
"%d:%d transaction failed %d, size %lld-%lld\n",
proc->pid, thread->pid, return_error,