linux-kernel - Re: [PATCH] binder: fix UAF in binder_netlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aXEFObeAwlzXprDC@google.com>
Date: Wed, 21 Jan 2026 16:56:25 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	Todd Kjos <tkjos@...roid.com>,
	Christian Brauner <brauner@...nel.org>, Li Li <dualli@...gle.com>,
	kernel-team@...roid.com, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH] binder: fix UAF in binder_netlink_report()

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