Skip to content

Commit d35d366

Browse files
toddkjosgregkh
authored andcommitted
binder: fix null deref of proc->context
The binder driver makes the assumption proc->context pointer is invariant after initialization (as documented in the kerneldoc header for struct proc). However, in commit f0fe2c0 ("binder: prevent UAF for binderfs devices II") proc->context is set to NULL during binder_deferred_release(). Another proc was in the middle of setting up a transaction to the dying process and crashed on a NULL pointer deref on "context" which is a local set to &proc->context: new_ref->data.desc = (node == context->binder_context_mgr_node) ? 0 : 1; Here's the stack: [ 5237.855435] Call trace: [ 5237.855441] binder_get_ref_for_node_olocked+0x100/0x2ec [ 5237.855446] binder_inc_ref_for_node+0x140/0x280 [ 5237.855451] binder_translate_binder+0x1d0/0x388 [ 5237.855456] binder_transaction+0x2228/0x3730 [ 5237.855461] binder_thread_write+0x640/0x25bc [ 5237.855466] binder_ioctl_write_read+0xb0/0x464 [ 5237.855471] binder_ioctl+0x30c/0x96c [ 5237.855477] do_vfs_ioctl+0x3e0/0x700 [ 5237.855482] __arm64_sys_ioctl+0x78/0xa4 [ 5237.855488] el0_svc_common+0xb4/0x194 [ 5237.855493] el0_svc_handler+0x74/0x98 [ 5237.855497] el0_svc+0x8/0xc The fix is to move the kfree of the binder_device to binder_free_proc() so the binder_device is freed when we know there are no references remaining on the binder_proc. Fixes: f0fe2c0 ("binder: prevent UAF for binderfs devices II") Acked-by: Christian Brauner <[email protected]> Signed-off-by: Todd Kjos <[email protected]> Cc: stable <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 4877846 commit d35d366

File tree

1 file changed

+7
-7
lines changed

1 file changed

+7
-7
lines changed

drivers/android/binder.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4686,8 +4686,15 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc)
46864686

46874687
static void binder_free_proc(struct binder_proc *proc)
46884688
{
4689+
struct binder_device *device;
4690+
46894691
BUG_ON(!list_empty(&proc->todo));
46904692
BUG_ON(!list_empty(&proc->delivered_death));
4693+
device = container_of(proc->context, struct binder_device, context);
4694+
if (refcount_dec_and_test(&device->ref)) {
4695+
kfree(proc->context->name);
4696+
kfree(device);
4697+
}
46914698
binder_alloc_deferred_release(&proc->alloc);
46924699
put_task_struct(proc->tsk);
46934700
binder_stats_deleted(BINDER_STAT_PROC);
@@ -5406,7 +5413,6 @@ static int binder_node_release(struct binder_node *node, int refs)
54065413
static void binder_deferred_release(struct binder_proc *proc)
54075414
{
54085415
struct binder_context *context = proc->context;
5409-
struct binder_device *device;
54105416
struct rb_node *n;
54115417
int threads, nodes, incoming_refs, outgoing_refs, active_transactions;
54125418

@@ -5423,12 +5429,6 @@ static void binder_deferred_release(struct binder_proc *proc)
54235429
context->binder_context_mgr_node = NULL;
54245430
}
54255431
mutex_unlock(&context->context_mgr_node_lock);
5426-
device = container_of(proc->context, struct binder_device, context);
5427-
if (refcount_dec_and_test(&device->ref)) {
5428-
kfree(context->name);
5429-
kfree(device);
5430-
}
5431-
proc->context = NULL;
54325432
binder_inner_proc_lock(proc);
54335433
/*
54345434
* Make sure proc stays alive after we

0 commit comments

Comments
 (0)