Skip to content

Commit

Permalink
FROMLIST: binder: fix UAF of ref->proc caused by race condition
Browse files Browse the repository at this point in the history
A transaction of type BINDER_TYPE_WEAK_HANDLE can fail to increment the
reference for a node. In this case, the target proc normally releases
the failed reference upon close as expected. However, if the target is
dying in parallel the call will race with binder_deferred_release(), so
the target could have released all of its references by now leaving the
cleanup of the new failed reference unhandled.

The transaction then ends and the target proc gets released making the
ref->proc now a dangling pointer. Later on, ref->node is closed and we
attempt to take spin_lock(&ref->proc->inner_lock), which leads to the
use-after-free bug reported below. Let's fix this by cleaning up the
failed reference on the spot instead of relying on the target to do so.

  ==================================================================
  BUG: KASAN: use-after-free in _raw_spin_lock+0xa8/0x150
  Write of size 4 at addr ffff5ca207094238 by task kworker/1:0/590

  CPU: 1 PID: 590 Comm: kworker/1:0 Not tainted 5.19.0-rc8 #10
  Hardware name: linux,dummy-virt (DT)
  Workqueue: events binder_deferred_func
  Call trace:
   dump_backtrace.part.0+0x1d0/0x1e0
   show_stack+0x18/0x70
   dump_stack_lvl+0x68/0x84
   print_report+0x2e4/0x61c
   kasan_report+0xa4/0x110
   kasan_check_range+0xfc/0x1a4
   __kasan_check_write+0x3c/0x50
   _raw_spin_lock+0xa8/0x150
   binder_deferred_func+0x5e0/0x9b0
   process_one_work+0x38c/0x5f0
   worker_thread+0x9c/0x694
   kthread+0x188/0x190
   ret_from_fork+0x10/0x20

Signed-off-by: Carlos Llamas <[email protected]>
Acked-by: Christian Brauner (Microsoft) <[email protected]>

Bug: 239630375
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Carlos Llamas <[email protected]>
Change-Id: I5085dd0dc805a780a64c057e5819f82dd8f02868
(cherry picked from commit ae3fa5d16a02ba7c7b170e0e1ab56d6f0ba33964)
  • Loading branch information
Carlos Llamas committed Aug 2, 2022
1 parent 33e4ebe commit 7c57e75
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions drivers/android/binder.c
Original file line number Diff line number Diff line change
Expand Up @@ -1942,6 +1942,18 @@ static int binder_inc_ref_for_node(struct binder_proc *proc,
}
ret = binder_inc_ref_olocked(ref, strong, target_list);
*rdata = ref->data;
if (ret && ref == new_ref) {
/*
* Cleanup the failed reference here as the target
* could now be dead and have already released its
* references by now. Calling on the new reference
* with strong=0 and a tmp_refs will not decrement
* the node. The new_ref gets kfree'd below.
*/
binder_cleanup_ref_olocked(new_ref);
ref = NULL;
}

binder_proc_unlock(proc);
if (new_ref && ref != new_ref)
/*
Expand Down

0 comments on commit 7c57e75

Please sign in to comment.