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

[Alice Ryhl <aliceryhl@google.com>]

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