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

[Carlos Llamas <cmllamas@google.com>]

On Wed, Jan 21, 2026 at 03:24:06PM +0000, Alice Ryhl wrote:
> 
> 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.

Right, the access to the target is safe because of the tmprefs just like
the rest of the transaction().

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

You actually can't access t->buffer already, there are scenarios where
the t->buffer is released before calling binder_netlink_report().

...

There is really nothing dangeours added by this solution. The fragile
part you mention comes from passing 't' to binder_netlink_report() in
the first place. As opposed to some static struct that contains all the
necessary info without potential issues. This is already present.

The ideal solution would be to refactor binder_transaction() to have a
central place where everything gets populated instead of having separate
objects for 'binder_transaction', 'binder_transaction_log_entry' and
'binder_extended_error'. All of them keep duplicated info and we don't
need more of them.

However, this is a larger effort that would require extensive testing as
we might introduce new issues, etc. I'm not sure that we even want to go
there. This solves the problem at hand so let's just fix it and move on.

--
Carlos Llamas