Re: [PATCH] binder: fix UAF when releasing todo list

On Fri, Oct 9, 2020 at 4:24 PM Todd Kjos tkjos@google.com wrote:

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() -> 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.

Signed-off-by: Todd Kjos tkjos@google.com

Cc: stable@vger.kernel.org # 4.14, 4.19, 5.4, 5.8

---

drivers/android/binder.c | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c index f936530a19b0..e8a1db4a86ed 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -223,7 +223,7 @@ static struct binder_transaction_log_entry *binder_transaction_log_add( struct binder_work { struct list_head entry;

•   enum {
  

•   enum binder_work_type {
            BINDER_WORK_TRANSACTION = 1,
            BINDER_WORK_TRANSACTION_COMPLETE,
            BINDER_WORK_RETURN_ERROR,
  

@@ -885,27 +885,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); @@ -4587,13 +4566,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;
  
  

@@ -4627,9 +4610,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;
            }
    }
  

-- 2.28.0.1011.ga647a8990f-goog