binder: fix UAF when releasing todo list

When releasing a thread todo list when tearing down
a binder_proc, the following race was possible which
could result in a use-after-free:

1.  Thread 1: enter binder_release_work from binder_thread_release
2.  Thread 2: binder_update_ref_for_handle() calls binder_dec_node_ilocked()
3.  Thread 2: dec nodeA --> 0 (will free node)
4.  Thread 1: ACQ inner_proc_lock
5.  Thread 2: block on inner_proc_lock
6.  Thread 1: dequeue work (BINDER_WORK_NODE, part of nodeA)
7.  Thread 1: REL inner_proc_lock
8.  Thread 2: ACQ inner_proc_lock
9.  Thread 2: todo list cleanup, but work was already dequeued
10. Thread 2: free node
11. Thread 2: REL inner_proc_lock
12. Thread 1: deref w->type (UAF)

The problem was that for a BINDER_WORK_NODE, the binder_work element
must not be accessed after releasing the inner_proc_lock while
processing the todo list elements since another thread might be
handling a deref on the node containing the binder_work element
leading to the node being freed.

Mot-CRs-fixed: (CR)
CVE-Fixed: CVE-2020-0423
Bug: 161151868

Signed-off-by: Todd Kjos <tkjos@google.com>
Change-Id: I4ae752abfe1aa38872be6f266ddd271802952625
Signed-off-by: Jignesh Patel <jignesh@motorola.com>
Reviewed-on: https://gerrit.mot.com/1747882
SLTApproved: Slta Waiver
SME-Granted: SME Approvals Granted
Tested-by: Jira Key
Reviewed-by: Xiangpo Zhao <zhaoxp3@motorola.com>
Submit-Approved: Jira Key
(cherry picked from commit dd0333429049cf16ce07df311dfcd5495416f12d)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index afa11e410d33ce8755480942f11797bd3c7ac495..3b9b6e2f5e248ce71b561b1e41670d744ee1c7c2 100644 (file)
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -283,7 +283,7 @@ struct binder_device {
 struct binder_work {
        struct list_head entry;
 
-       enum {
+       enum binder_work_type {
                BINDER_WORK_TRANSACTION = 1,
                BINDER_WORK_TRANSACTION_COMPLETE,
                BINDER_WORK_RETURN_ERROR,
@@ -1011,27 +1011,6 @@ static struct binder_work *binder_dequeue_work_head_ilocked(
        return w;
 }
 
-/**
- * binder_dequeue_work_head() - Dequeues the item at head of list
- * @proc:         binder_proc associated with list
- * @list:         list to dequeue head
- *
- * Removes the head of the list if there are items on the list
- *
- * Return: pointer dequeued binder_work, NULL if list was empty
- */
-static struct binder_work *binder_dequeue_work_head(
-                                       struct binder_proc *proc,
-                                       struct list_head *list)
-{
-       struct binder_work *w;
-
-       binder_inner_proc_lock(proc);
-       w = binder_dequeue_work_head_ilocked(list);
-       binder_inner_proc_unlock(proc);
-       return w;
-}
-
 static void
 binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer);
 static void binder_free_thread(struct binder_thread *thread);
@@ -4551,13 +4530,17 @@ static void binder_release_work(struct binder_proc *proc,
                                struct list_head *list)
 {
        struct binder_work *w;
+       enum binder_work_type wtype;
 
        while (1) {
-               w = binder_dequeue_work_head(proc, list);
+               binder_inner_proc_lock(proc);
+               w = binder_dequeue_work_head_ilocked(list);
+               wtype = w ? w->type : 0;
+               binder_inner_proc_unlock(proc);
                if (!w)
                        return;
 
-               switch (w->type) {
+               switch (wtype) {
                case BINDER_WORK_TRANSACTION: {
                        struct binder_transaction *t;
 
@@ -4591,9 +4574,11 @@ static void binder_release_work(struct binder_proc *proc,
                        kfree(death);
                        binder_stats_deleted(BINDER_STAT_DEATH);
                } break;
+               case BINDER_WORK_NODE:
+                       break;
                default:
                        pr_err("unexpected work type, %d, not freed\n",
-                              w->type);
+                              wtype);
                        break;
                }
        }