Re: [PATCH] binder: fix UAF in binder_netlink_report()

[Alice Ryhl <aliceryhl@google.com>]

On Wed, Jan 21, 2026 at 02:50:04PM +0000, Carlos Llamas wrote:
> Oneway transactions sent to frozen targets via binder_proc_transaction()
> return a BR_TRANSACTION_PENDING_FROZEN error but they are still treated
> as successful since the target is expected to thaw at some point. It is
> then not safe to access 't' after BR_TRANSACTION_PENDING_FROZEN errors
> as the transaction could have been consumed by the now thawed target.
> 
> This is the case for binder_netlink_report() which derreferences 't'
> after a pending frozen error, as pointed out by the following KASAN
> report:
> 
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in binder_netlink_report.isra.0+0x694/0x6c8
>   Read of size 8 at addr ffff00000f98ba38 by task binder-util/522
> 
>   CPU: 4 UID: 0 PID: 522 Comm: binder-util Not tainted 6.19.0-rc6-00015-gc03e9c42ae8f #1 PREEMPT
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    binder_netlink_report.isra.0+0x694/0x6c8
>    binder_transaction+0x66e4/0x79b8
>    binder_thread_write+0xab4/0x4440
>    binder_ioctl+0x1fd4/0x2940
>    [...]
> 
>   Allocated by task 522:
>    __kmalloc_cache_noprof+0x17c/0x50c
>    binder_transaction+0x584/0x79b8
>    binder_thread_write+0xab4/0x4440
>    binder_ioctl+0x1fd4/0x2940
>    [...]
> 
>   Freed by task 488:
>    kfree+0x1d0/0x420
>    binder_free_transaction+0x150/0x234
>    binder_thread_read+0x2d08/0x3ce4
>    binder_ioctl+0x488/0x2940
>    [...]
>   ==================================================================
> 
> Instead, make a transaction copy so the data can be safely accessed by
> binder_netlink_report() after a pending frozen error.
> 
> Cc: stable@vger.kernel.org
> Fixes: 63740349eba7 ("binder: introduce transaction reports via netlink")
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 535fc881c8da..70dc63a6e06a 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3780,6 +3780,14 @@ static void binder_transaction(struct binder_proc *proc,
>  			goto err_dead_proc_or_thread;
>  		}
>  	} else {
> +		/*
> +		 * Make a transaction copy. It is not safe to access 't' after
> +		 * binder_proc_transaction() reported a pending frozen. The
> +		 * target could thaw and consume the transaction at any point.
> +		 * Instead, use a safe 't_copy' for binder_netlink_report().
> +		 */
> +		struct binder_transaction t_copy = *t;
> +
>  		BUG_ON(target_node == NULL);
>  		BUG_ON(t->buffer->async_transaction != 1);
>  		return_error = binder_proc_transaction(t, target_proc, NULL);
> @@ -3790,7 +3798,7 @@ static void binder_transaction(struct binder_proc *proc,
>  		 */
>  		if (return_error == BR_TRANSACTION_PENDING_FROZEN) {
>  			tcomplete->type = BINDER_WORK_TRANSACTION_PENDING;
> -			binder_netlink_report(proc, t, tr->data_size,
> +			binder_netlink_report(proc, &t_copy, tr->data_size,

Erm, this solution seems dangerous to me. You access t->to_proc and
t->to_thread inside binder_netlink_report(), and if t has been freed,
could the same apply to t->to_proc or t->to_thread?

After looking a bit more: I can see now that you do call

	if (target_thread)
		binder_thread_dec_tmpref(target_thread);
	binder_proc_dec_tmpref(target_proc);
	if (target_node)
		binder_dec_node_tmpref(target_node);

after this ... so I guess it can't go wrong in this particular way.

But I'm concerned that we will add fields in the future where this is
not the case. For example, let's say that tomorrow I want to include
t->buffer->clear_on_free in the printed data. If the transaction is
freed, then t->buffer might also be freed.

Alice