[PATCH v2 2/2] binder: free transaction buffer via binder_free_buf() on read -EFAULT

[Hui Peng]

In binder_thread_read(), if put_user() or copy_to_user() returns -EFAULT,
the failure paths call binder_cleanup_transaction() without setting
t->buffer->transaction = NULL or calling binder_free_buf(proc, thread,
buffer, true).

Because allow_user_free remains 0, the binder_buffer and all translated
binder_node / binder_ref references inside it are permanently leaked. For
oneway transactions (TF_ONE_WAY), node->has_async_transaction remains true
forever, wedging all subsequent async transactions to that node.

Fix this by setting buffer->transaction = NULL and calling
binder_free_buf(proc, thread, buffer, true) on put_user() / copy_to_user()
-EFAULT errors in binder_thread_read().

Tested in QEMU against Linux 7.3.0-rc3 using a multi-process C reproducer:
triggering copy_to_user() failure on a one-way transaction: on the unfixed
kernel, binder_cleanup_transaction() leaked the transaction buffer and
left node->has_async_transaction set to true (wedging subsequent async
transactions); whereas with this fix applied, binder_free_buf() is called
on -EFAULT, clearing buffer->transaction and freeing the allocation cleanly.

Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds")
Cc: stable@xxxxxxxxxxxxxxx
Reviewed-by: Carlos Llamas <cmllamas@xxxxxxxxxx>
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
---
Changes in v2:
- Split out as patch 2/2 as requested by Greg Kroah-Hartman and Carlos
  Llamas.
- Added QEMU test procedure details in commit message body.
- Added Reviewed-by tag from Carlos Llamas.

 drivers/android/binder.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ee01e6a06631..efff59853752 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5127,24 +5128,32 @@ static int binder_thread_read(struct binder_proc *proc,
 			trsize = sizeof(tr);
 		}
 		if (put_user(cmd, (uint32_t __user *)ptr)) {
+			struct binder_buffer *buffer = t->buffer;
+
 			if (t_from)
 				binder_thread_dec_tmpref(t_from);
 
+			buffer->transaction = NULL;
 			binder_cleanup_transaction(t, "put_user failed",
 						   BR_FAILED_REPLY);
+			binder_free_buf(proc, thread, buffer, true);
 
 			return -EFAULT;
 		}
 		ptr += sizeof(uint32_t);
 		if (copy_to_user(ptr, &tr, trsize)) {
+			struct binder_buffer *buffer = t->buffer;
+
 			if (t_from)
 				binder_thread_dec_tmpref(t_from);
 
+			buffer->transaction = NULL;
 			binder_cleanup_transaction(t, "copy_to_user failed",
 						   BR_FAILED_REPLY);
+			binder_free_buf(proc, thread, buffer, true);
 
 			return -EFAULT;
 		}
 		ptr += trsize;