The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH] binder: fix UAF in binder_thread_release()
@ 2026-06-06  2:22 Carlos Llamas
  2026-06-10  6:50 ` Alice Ryhl
  0 siblings, 1 reply; 2+ messages in thread
From: Carlos Llamas @ 2026-06-06  2:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
	Christian Brauner, Carlos Llamas, Alice Ryhl
  Cc: kernel-team, linux-kernel, stable

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")
Signed-off-by: Carlos Llamas <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.54.0.1032.g2f8565e1d1-goog


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] binder: fix UAF in binder_thread_release()
  2026-06-06  2:22 [PATCH] binder: fix UAF in binder_thread_release() Carlos Llamas
@ 2026-06-10  6:50 ` Alice Ryhl
  0 siblings, 0 replies; 2+ messages in thread
From: Alice Ryhl @ 2026-06-10  6:50 UTC (permalink / raw)
  To: Carlos Llamas
  Cc: Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
	Christian Brauner, kernel-team, linux-kernel, stable

On Sat, Jun 06, 2026 at 02:22:32AM +0000, Carlos Llamas wrote:
> 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")
> Signed-off-by: Carlos Llamas <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);

Although I don't think this fixes all issues here, as we discussed more
in private, this does fix the specific UAF referenced in this patch, so:

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

The logic is that either binder_free_transaction() reads a non-null
target_proc, in which case we take the inner proc lock and fully
synchronize with the entirety of binder_thread_release(), or we read a
null target_proc in which case the transaction spinlock ensures that we
wait for the part of binder_thread_release() touching this particular
'struct binder_transaction'.

Alice

>  	if (target_proc) {
>  		binder_inner_proc_lock(target_proc);
> -- 
> 2.54.0.1032.g2f8565e1d1-goog
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-06-10  6:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-06  2:22 [PATCH] binder: fix UAF in binder_thread_release() Carlos Llamas
2026-06-10  6:50 ` Alice Ryhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox