drivers/android/binder.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
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,
return_error);
}
binder_enqueue_thread_work(thread, tcomplete);
--
2.52.0.457.g6b5491de43-googOn 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
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
On Wed, Jan 21, 2026 at 04:56:25PM +0000, Carlos Llamas wrote: > 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(). Hmm, I suppose you are right. It may be worth mentioning that you can't access t->buffer in a comment inside netlink_report? Alice
On Thu, Jan 22, 2026 at 08:27:13AM +0000, Alice Ryhl wrote: > On Wed, Jan 21, 2026 at 04:56:25PM +0000, Carlos Llamas wrote: > > 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(). > > Hmm, I suppose you are right. It may be worth mentioning that you can't > access t->buffer in a comment inside netlink_report? ok, that is a good idea. I'll send a v2.
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. While here, add a
comment about not using t->buffer in binder_netlink_report().
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 | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 535fc881c8da..4e16438eceb7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2991,6 +2991,10 @@ static void binder_set_txn_from_error(struct binder_transaction *t, int id,
* @t: the binder transaction that failed
* @data_size: the user provided data size for the transaction
* @error: enum binder_driver_return_protocol returned to sender
+ *
+ * Note that t->buffer is not safe to access here, as it may have been
+ * released (or not yet allocated). Callers should guarantee all the
+ * transaction items used here are safe to access.
*/
static void binder_netlink_report(struct binder_proc *proc,
struct binder_transaction *t,
@@ -3780,6 +3784,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 +3802,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,
return_error);
}
binder_enqueue_thread_work(thread, tcomplete);
--
2.52.0.457.g6b5491de43-googOn Thu, Jan 22, 2026 at 06:02:02PM +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. While here, add a
> comment about not using t->buffer in binder_netlink_report().
>
> Cc: stable@vger.kernel.org
> Fixes: 63740349eba7 ("binder: introduce transaction reports via netlink")
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
© 2016 - 2026 Red Hat, Inc.