binder: binder: fix possible UAF when freeing buffer
There is a race between the binder driver cleaning
up a completed transaction via binder_free_transaction()
and a user calling binder_ioctl(BC_FREE_BUFFER) to
release a buffer. It doesn't matter which is first but
they need to be protected against running concurrently
which can result in a UAF.
Bug: 133758011
Change-Id: Ie1426ff3d00218d050d61ff77b333ddf8818b7c9
Signed-off-by: Todd Kjos <tkjos@google.com>
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index a3c0f5f..39f588b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -528,7 +528,8 @@
* @requested_threads_started: number binder threads started
* (protected by @inner_lock)
* @tmp_ref: temporary reference to indicate proc is in use
- * (protected by @inner_lock)
+ * (atomic since @proc->inner_lock cannot
+ * always be acquired)
* @default_priority: default scheduler priority
* (invariant after initialized)
* @debugfs_entry: debugfs node
@@ -562,7 +563,7 @@
int max_threads;
int requested_threads;
int requested_threads_started;
- int tmp_ref;
+ atomic_t tmp_ref;
struct binder_priority default_priority;
struct dentry *debugfs_entry;
struct binder_alloc alloc;
@@ -2053,9 +2054,9 @@
static void binder_proc_dec_tmpref(struct binder_proc *proc)
{
binder_inner_proc_lock(proc);
- proc->tmp_ref--;
+ atomic_dec(&proc->tmp_ref);
if (proc->is_dead && RB_EMPTY_ROOT(&proc->threads) &&
- !proc->tmp_ref) {
+ !atomic_read(&proc->tmp_ref)) {
binder_inner_proc_unlock(proc);
binder_free_proc(proc);
return;
@@ -2117,8 +2118,26 @@
static void binder_free_transaction(struct binder_transaction *t)
{
- if (t->buffer)
- t->buffer->transaction = NULL;
+ struct binder_proc *target_proc;
+
+ spin_lock(&t->lock);
+ target_proc = t->to_proc;
+ if (target_proc) {
+ atomic_inc(&target_proc->tmp_ref);
+ spin_unlock(&t->lock);
+
+ binder_inner_proc_lock(target_proc);
+ if (t->buffer)
+ t->buffer->transaction = NULL;
+ binder_inner_proc_unlock(target_proc);
+ binder_proc_dec_tmpref(target_proc);
+ } else {
+ /*
+ * If the transaction has no target_proc, then
+ * t->buffer->transaction * has already been cleared.
+ */
+ spin_unlock(&t->lock);
+ }
kfree(t);
binder_stats_deleted(BINDER_STAT_TRANSACTION);
}
@@ -2871,7 +2890,7 @@
target_node = node;
binder_inc_node_nilocked(node, 1, 0, NULL);
binder_inc_node_tmpref_ilocked(node);
- node->proc->tmp_ref++;
+ atomic_inc(&node->proc->tmp_ref);
*procp = node->proc;
} else
*error = BR_DEAD_REPLY;
@@ -2967,7 +2986,7 @@
goto err_dead_binder;
}
target_proc = target_thread->proc;
- target_proc->tmp_ref++;
+ atomic_inc(&target_proc->tmp_ref);
binder_inner_proc_unlock(target_thread->proc);
} else {
if (tr->target.handle) {
@@ -3700,10 +3719,12 @@
buffer->debug_id,
buffer->transaction ? "active" : "finished");
+ binder_inner_proc_lock(proc);
if (buffer->transaction) {
buffer->transaction->buffer = NULL;
buffer->transaction = NULL;
}
+ binder_inner_proc_unlock(proc);
if (buffer->async_transaction && buffer->target_node) {
struct binder_node *buf_node;
struct binder_work *w;
@@ -4565,7 +4586,7 @@
* The corresponding dec is when we actually
* free the thread in binder_free_thread()
*/
- proc->tmp_ref++;
+ atomic_inc(&proc->tmp_ref);
/*
* take a ref on this thread to ensure it
* survives while we are releasing it
@@ -5005,6 +5026,7 @@
return -ENOMEM;
spin_lock_init(&proc->inner_lock);
spin_lock_init(&proc->outer_lock);
+ atomic_set(&proc->tmp_ref, 0);
get_task_struct(current->group_leader);
proc->tsk = current->group_leader;
mutex_init(&proc->files_lock);
@@ -5184,7 +5206,7 @@
* Make sure proc stays alive after we
* remove all the threads
*/
- proc->tmp_ref++;
+ atomic_inc(&proc->tmp_ref);
proc->is_dead = true;
threads = 0;
