[PATCH v2 1/2] binder: fix UAF in binder_thread_release()

[PATCH v2 1/2] binder: fix UAF in binder_thread_release()

[Carlos Llamas]

When a thread exits, binder_thread_release() walks its transaction stack
to clear the t->from and t->to_proc that correspond with the exiting
thread. However, a process dying in parallel might attempt to kfree some
of these transactions. And if one of them has no associated t->to_proc,
the t->to_proc->inner_lock will not be acquired.

This means that transaction accesses in binder_thread_release() after
t->to_proc has been cleared might race with binder_free_transaction()
and cause a use-after-free error as reported by KASAN:

  ==================================================================
  BUG: KASAN: slab-use-after-free in binder_thread_release+0x5d0/0x798
  Write of size 8 at addr ffff000016627500 by task X/715

  CPU: 17 UID: 0 PID: 715 Comm: X Not tainted 7.1.0-rc5-00149-g8fde5d1d47f6 #30 PREEMPT
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   binder_thread_release+0x5d0/0x798
   binder_ioctl+0x12c0/0x299c
   [...]

  Allocated by task 717 on cpu 18 at 67.267803s:
   __kasan_kmalloc+0xa0/0xbc
   __kmalloc_cache_noprof+0x174/0x444
   binder_transaction+0x554/0x8150
   binder_thread_write+0xa30/0x4354
   binder_ioctl+0x20f0/0x299c
   [...]

  Freed by task 202 on cpu 18 at 90.416221s:
   __kasan_slab_free+0x58/0x80
   kfree+0x1a0/0x4a4
   binder_free_transaction+0x150/0x294
   binder_send_failed_reply+0x398/0x6d8
   binder_release_work+0x3e4/0x4ec
   binder_deferred_func+0xbd8/0x104c
   [...]
  ==================================================================

In order to avoid this, make sure that binder_free_transaction() reads
the t->to_proc under the transaction lock. This will serialize the
transaction release with the accesses in binder_thread_release(). Plus,
it matches the documented locking rules for @to_proc.

Cc: stable@vger.kernel.org
Fixes: 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe")
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
v2:
 - Collected RB tag from Alice.
 - Attached a new patch [2/2] to fix a separate vulnerability reported
   by Alice. 

v1:
 https://lore.kernel.org/all/20260606022233.2402965-1-cmllamas@google.com/

 drivers/android/binder.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9e6194224593..09bc052186cf 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1658,7 +1658,11 @@ static void binder_txn_latency_free(struct binder_transaction *t)
 
 static void binder_free_transaction(struct binder_transaction *t)
 {
-	struct binder_proc *target_proc = t->to_proc;
+	struct binder_proc *target_proc;
+
+	spin_lock(&t->lock);
+	target_proc = t->to_proc;
+	spin_unlock(&t->lock);
 
 	if (target_proc) {
 		binder_inner_proc_lock(target_proc);
-- 
2.55.0.rc0.738.g0c8ab3ebcc-goog

[PATCH v2 2/2] binder: fix UAF in binder_free_transaction()

[Carlos Llamas]

In binder_free_transaction(), the t->to_proc is read under the t->lock.
However, once the t->lock is dropped, the to_proc can die in parallel.
This leads to a use-after-free error when we attempt to acquire its
inner lock right afterwards:

  ==================================================================
  BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xe4/0x1a0
  Write of size 4 at addr ffff00001125da70 by task B/672

  CPU: 20 UID: 0 PID: 672 Comm: B Not tainted 7.1.0-rc6-00284-g8e65320d91cd #4 PREEMPT
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   _raw_spin_lock+0xe4/0x1a0
   binder_free_transaction+0x8c/0x320
   binder_send_failed_reply+0x21c/0x2f8
   binder_thread_release+0x488/0x7e0
   binder_ioctl+0x12c0/0x29a0
  [...]

  Allocated by task 675:
   __kmalloc_cache_noprof+0x174/0x444
   binder_open+0x118/0xb70
   do_dentry_open+0x374/0x1040
   vfs_open+0x58/0x3bc
  [...]

  Freed by task 212:
   __kasan_slab_free+0x58/0x80
   kfree+0x1a0/0x4a4
   binder_proc_dec_tmpref+0x32c/0x5e0
   binder_deferred_func+0xc48/0x104c
   process_one_work+0x53c/0xbc0
  [...]
  ==================================================================

To prevent this, pin the target thread (t->to_thread) to guarantee the
target process remains alive. Undelivered transactions without a target
thread are already safe, as the target process can only be the current
context in those paths.

Cc: stable@vger.kernel.org
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/all/aikJKVuny_eOivwN@google.com/
Fixes: a370003cc301 ("binder: fix possible UAF when freeing buffer")
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 09bc052186cf..b85920c39694 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1658,10 +1658,19 @@ static void binder_txn_latency_free(struct binder_transaction *t)
 
 static void binder_free_transaction(struct binder_transaction *t)
 {
+	struct binder_thread *target_thread;
 	struct binder_proc *target_proc;
 
 	spin_lock(&t->lock);
 	target_proc = t->to_proc;
+	target_thread = t->to_thread;
+	/*
+	 * Pin target_thread to keep target_proc alive. Undelivered
+	 * transactions with !target_thread are safe, as target_proc
+	 * can only be the current context there.
+	 */
+	if (target_thread)
+		atomic_inc(&target_thread->tmp_ref);
 	spin_unlock(&t->lock);
 
 	if (target_proc) {
@@ -1676,6 +1685,10 @@ static void binder_free_transaction(struct binder_transaction *t)
 			t->buffer->transaction = NULL;
 		binder_inner_proc_unlock(target_proc);
 	}
+
+	if (target_thread)
+		binder_thread_dec_tmpref(target_thread);
+
 	if (trace_binder_txn_latency_free_enabled())
 		binder_txn_latency_free(t);
 	/*
-- 
2.55.0.rc0.738.g0c8ab3ebcc-goog

Re: [PATCH v2 2/2] binder: fix UAF in binder_free_transaction()

[Alice Ryhl]

On Fri, Jun 19, 2026 at 8:52 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> In binder_free_transaction(), the t->to_proc is read under the t->lock.
> However, once the t->lock is dropped, the to_proc can die in parallel.
> This leads to a use-after-free error when we attempt to acquire its
> inner lock right afterwards:
>
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xe4/0x1a0
>   Write of size 4 at addr ffff00001125da70 by task B/672
>
>   CPU: 20 UID: 0 PID: 672 Comm: B Not tainted 7.1.0-rc6-00284-g8e65320d91cd #4 PREEMPT
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    _raw_spin_lock+0xe4/0x1a0
>    binder_free_transaction+0x8c/0x320
>    binder_send_failed_reply+0x21c/0x2f8
>    binder_thread_release+0x488/0x7e0
>    binder_ioctl+0x12c0/0x29a0
>   [...]
>
>   Allocated by task 675:
>    __kmalloc_cache_noprof+0x174/0x444
>    binder_open+0x118/0xb70
>    do_dentry_open+0x374/0x1040
>    vfs_open+0x58/0x3bc
>   [...]
>
>   Freed by task 212:
>    __kasan_slab_free+0x58/0x80
>    kfree+0x1a0/0x4a4
>    binder_proc_dec_tmpref+0x32c/0x5e0
>    binder_deferred_func+0xc48/0x104c
>    process_one_work+0x53c/0xbc0
>   [...]
>   ==================================================================
>
> To prevent this, pin the target thread (t->to_thread) to guarantee the
> target process remains alive. Undelivered transactions without a target
> thread are already safe, as the target process can only be the current
> context in those paths.
>
> Cc: stable@vger.kernel.org
> Reported-by: Alice Ryhl <aliceryhl@google.com>
> Closes: https://lore.kernel.org/all/aikJKVuny_eOivwN@google.com/
> Fixes: a370003cc301 ("binder: fix possible UAF when freeing buffer")
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 09bc052186cf..b85920c39694 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1658,10 +1658,19 @@ static void binder_txn_latency_free(struct binder_transaction *t)
>
>  static void binder_free_transaction(struct binder_transaction *t)
>  {
> +       struct binder_thread *target_thread;
>         struct binder_proc *target_proc;
>
>         spin_lock(&t->lock);
>         target_proc = t->to_proc;
> +       target_thread = t->to_thread;
> +       /*
> +        * Pin target_thread to keep target_proc alive. Undelivered
> +        * transactions with !target_thread are safe, as target_proc
> +        * can only be the current context there.
> +        */
> +       if (target_thread)
> +               atomic_inc(&target_thread->tmp_ref);

This is more complicated than the comment suggests, but I think it's
correct. As far as I can tell, scenarios where to_thread is NULL but
to_proc is not are also scenarios where the caller ensures that
to_proc stays alive during this function call.

It's unfortunate that there's no obvious better way of doing this. I'd
like to just take a refcount on the process, but it's not atomic, and
you can't take the proc lock protecting it because of lock inversion.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Alice

Re: [PATCH v2 2/2] binder: fix UAF in binder_free_transaction()

[Carlos Llamas]

On Mon, Jun 22, 2026 at 09:55:29PM +0200, Alice Ryhl wrote:
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -1658,10 +1658,19 @@ static void binder_txn_latency_free(struct binder_transaction *t)
> >
> >  static void binder_free_transaction(struct binder_transaction *t)
> >  {
> > +       struct binder_thread *target_thread;
> >         struct binder_proc *target_proc;
> >
> >         spin_lock(&t->lock);
> >         target_proc = t->to_proc;
> > +       target_thread = t->to_thread;
> > +       /*
> > +        * Pin target_thread to keep target_proc alive. Undelivered
> > +        * transactions with !target_thread are safe, as target_proc
> > +        * can only be the current context there.
> > +        */
> > +       if (target_thread)
> > +               atomic_inc(&target_thread->tmp_ref);
> 
> This is more complicated than the comment suggests, but I think it's
> correct. As far as I can tell, scenarios where to_thread is NULL but
> to_proc is not are also scenarios where the caller ensures that
> to_proc stays alive during this function call.

Right, transactions with !to_thread and a valid to_proc are undelivered,
and as such they can only reach this point from the to_proc's context.

> 
> It's unfortunate that there's no obvious better way of doing this. I'd
> like to just take a refcount on the process, but it's not atomic, and
> you can't take the proc lock protecting it because of lock inversion.

Bingo! I'm using the thread as a proxy because refactoring proc->tmp_ref
to be atomic would be a bigger and thus riskier change.

> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Thanks!