binder: fix buffer/node leak and premature fd_install on read -EFAULT

[PATCH] binder: fix buffer/node leak and premature fd_install on read -EFAULT

Posted by Hui Peng 4 days, 23 hours ago

In `binder_thread_read()`, `binder_apply_fd_fixups()` installs
translated file descriptors into `current->files` via `fd_install()`
BEFORE `put_user()` and `copy_to_user()` copy the `BR_TRANSACTION` /
`BR_REPLY` command and `binder_transaction_data` payload to userspace.

If `put_user()` or `copy_to_user()` returns `-EFAULT`:

1. Any file descriptors in `t->fd_fixups` have already been installed
   into the recipient process's file descriptor table and removed from
   `t->fd_fixups`, even though the transaction failed and the sender
   receives `BR_FAILED_REPLY`.
2. Unlike the `binder_apply_fd_fixups()` failure path immediately above
   it, the `put_user()` and `copy_to_user()` 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` is still `0`, the
   `binder_buffer` and all translated `binder_node`/`binder_ref`
   references inside it are permanently leaked, and for `TF_ONE_WAY`
   transactions `node->has_async_transaction` remains `true` forever,
   wedging all subsequent async transactions to that node.

Split `fd_install()` out of `binder_apply_fd_fixups()` into
`binder_fd_fixups_install(t)` called only after `put_user()` and
`copy_to_user()` succeed, and free `t->buffer` via
`binder_free_buf(proc, thread, buffer, true)` on `-EFAULT`.

Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>

---
 drivers/android/binder.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fb9ff072bd6c..efff59853752 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4705,7 +4705,7 @@ static int binder_wait_for_work(struct binder_thread *thread,
 static int binder_apply_fd_fixups(struct binder_proc *proc,
 				  struct binder_transaction *t)
 {
-	struct binder_txn_fd_fixup *fixup, *tmp;
+	struct binder_txn_fd_fixup *fixup;
 	int ret = 0;
 
 	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
@@ -4730,19 +4730,25 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
 			goto err;
 		}
 	}
-	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
-		fd_install(fixup->target_fd, fixup->file);
-		list_del(&fixup->fixup_entry);
-		kfree(fixup);
-	}
 
-	return ret;
+	return 0;
 
 err:
 	binder_free_txn_fixups(t);
 	return ret;
 }
 
+static void binder_fd_fixups_install(struct binder_transaction *t)
+{
+	struct binder_txn_fd_fixup *fixup, *tmp;
+
+	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
+		fd_install(fixup->target_fd, fixup->file);
+		list_del(&fixup->fixup_entry);
+		kfree(fixup);
+	}
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      binder_uintptr_t binder_buffer, size_t size,
@@ -5121,26 +5127,36 @@ 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;
 
+		binder_fd_fixups_install(t);
+
 		trace_binder_transaction_received(t);
 		binder_stat_br(proc, thread, cmd);
 		binder_debug(BINDER_DEBUG_TRANSACTION,
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v2 0/2] binder: fix premature fd_install() and buffer leak on read -EFAULT

Posted by Hui Peng 11 hours ago

This series fixes two error-handling issues in binder_thread_read():

1. Defer fd_install() until after put_user() and copy_to_user() succeed,
   preventing file descriptors from being prematurely installed into the
   recipient process's file descriptor table when userspace copy fails.
2. Call binder_free_buf() on -EFAULT error paths in binder_thread_read() so
   the transaction buffer and associated node/ref references are freed.

Changes in v2:
- Split into a 2-patch series with separate single-purpose commits as
  requested by Greg Kroah-Hartman and Carlos Llamas.
- Added Reviewed-by tag from Carlos Llamas.

Hui Peng (2):
  binder: defer fd_install() until after copy_to_user() in binder_thread_read()
  binder: free transaction buffer via binder_free_buf() on read -EFAULT

 drivers/android/binder.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

Re: [PATCH] binder: fix buffer/node leak and premature fd_install on read -EFAULT

Posted by Carlos Llamas 22 hours ago

On Sat, Sep 19, 2026 at 09:36:49PM +0000, Hui Peng wrote:
> In `binder_thread_read()`, `binder_apply_fd_fixups()` installs
> translated file descriptors into `current->files` via `fd_install()`
> BEFORE `put_user()` and `copy_to_user()` copy the `BR_TRANSACTION` /
> `BR_REPLY` command and `binder_transaction_data` payload to userspace.
> 
> If `put_user()` or `copy_to_user()` returns `-EFAULT`:
> 
> 1. Any file descriptors in `t->fd_fixups` have already been installed
>    into the recipient process's file descriptor table and removed from
>    `t->fd_fixups`, even though the transaction failed and the sender
>    receives `BR_FAILED_REPLY`.
> 2. Unlike the `binder_apply_fd_fixups()` failure path immediately above
>    it, the `put_user()` and `copy_to_user()` 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` is still `0`, the
>    `binder_buffer` and all translated `binder_node`/`binder_ref`
>    references inside it are permanently leaked, and for `TF_ONE_WAY`
>    transactions `node->has_async_transaction` remains `true` forever,
>    wedging all subsequent async transactions to that node.
> 
> Split `fd_install()` out of `binder_apply_fd_fixups()` into
> `binder_fd_fixups_install(t)` called only after `put_user()` and
> `copy_to_user()` succeed, and free `t->buffer` via
> `binder_free_buf(proc, thread, buffer, true)` on `-EFAULT`.
> 
> Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> 
> ---
>  drivers/android/binder.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index fb9ff072bd6c..efff59853752 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -4705,7 +4705,7 @@ static int binder_wait_for_work(struct binder_thread *thread,
>  static int binder_apply_fd_fixups(struct binder_proc *proc,
>  				  struct binder_transaction *t)
>  {
> -	struct binder_txn_fd_fixup *fixup, *tmp;
> +	struct binder_txn_fd_fixup *fixup;
>  	int ret = 0;
>  
>  	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
> @@ -4730,19 +4730,25 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
>  			goto err;
>  		}
>  	}
> -	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
> -		fd_install(fixup->target_fd, fixup->file);
> -		list_del(&fixup->fixup_entry);
> -		kfree(fixup);
> -	}
>  
> -	return ret;
> +	return 0;
>  
>  err:
>  	binder_free_txn_fixups(t);
>  	return ret;
>  }
>  
> +static void binder_fd_fixups_install(struct binder_transaction *t)
> +{
> +	struct binder_txn_fd_fixup *fixup, *tmp;
> +
> +	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
> +		fd_install(fixup->target_fd, fixup->file);
> +		list_del(&fixup->fixup_entry);
> +		kfree(fixup);
> +	}
> +}
> +
>  static int binder_thread_read(struct binder_proc *proc,
>  			      struct binder_thread *thread,
>  			      binder_uintptr_t binder_buffer, size_t size,
> @@ -5121,26 +5127,36 @@ 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;
>  
> +		binder_fd_fixups_install(t);
> +
>  		trace_binder_transaction_received(t);
>  		binder_stat_br(proc, thread, cmd);
>  		binder_debug(BINDER_DEBUG_TRANSACTION,
> -- 
> 2.55.0.1082.g2b9226bbc0-goog
> 

This seems correct. Although I assume you'll be sending a new patchset
addressing Greg's feedback right? e.g.

https://lore.kernel.org/all/2026092022-wad-vertigo-3361@gregkh/

[PATCH v2 1/2] binder: defer fd_install() until after copy_to_user() in binder_thread_read()

Posted by Hui Peng 11 hours ago

In binder_thread_read(), binder_apply_fd_fixups() installs translated file
descriptors into current->files via fd_install() BEFORE put_user() and
copy_to_user() copy the BR_TRANSACTION / BR_REPLY command to userspace.

If put_user() or copy_to_user() returns -EFAULT, any file descriptors in
t->fd_fixups have already been installed into the recipient process's file
descriptor table and removed from t->fd_fixups, even though the transaction
failed and the sender receives BR_FAILED_REPLY.

Split fd_install() out of binder_apply_fd_fixups() into
binder_fd_fixups_install(t) and invoke it only after put_user() and
copy_to_user() succeed.

Tested in QEMU against Linux 7.3.0-rc3 using a multi-process C reproducer:
a sender thread transmits a pipe write-end FD via BINDER_TYPE_FD in a
one-way transaction to a recipient thread whose read buffer spans an
unmapped virtual memory page, triggering -EFAULT in copy_to_user(): on the
unfixed kernel, the pipe write-end FD was prematurely installed into
current->files (polling the read-end did NOT produce POLLHUP); whereas
with this fix applied, fd_install() is deferred past copy_to_user(), so
the FD is not installed on -EFAULT (polling the read-end produces POLLHUP).

Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds")
Cc: stable@vger.kernel.org
Reviewed-by: Carlos Llamas <cmllamas@google.com>
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split out as patch 1/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 | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fb9ff072bd6c..ee01e6a06631 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4705,7 +4705,7 @@ static int binder_wait_for_work(struct binder_thread *thread,
 static int binder_apply_fd_fixups(struct binder_proc *proc,
 				  struct binder_transaction *t)
 {
-	struct binder_txn_fd_fixup *fixup, *tmp;
+	struct binder_txn_fd_fixup *fixup;
 	int ret = 0;
 
 	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
@@ -4730,19 +4730,25 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
 			goto err;
 		}
 	}
-	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
-		fd_install(fixup->target_fd, fixup->file);
-		list_del(&fixup->fixup_entry);
-		kfree(fixup);
-	}
 
-	return ret;
+	return 0;
 
 err:
 	binder_free_txn_fixups(t);
 	return ret;
 }
 
+static void binder_fd_fixups_install(struct binder_transaction *t)
+{
+	struct binder_txn_fd_fixup *fixup, *tmp;
+
+	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
+		fd_install(fixup->target_fd, fixup->file);
+		list_del(&fixup->fixup_entry);
+		kfree(fixup);
+	}
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      binder_uintptr_t binder_buffer, size_t size,
@@ -5141,6 +5147,8 @@ static int binder_thread_read(struct binder_proc *proc,
 		}
 		ptr += trsize;
 
+		binder_fd_fixups_install(t);
+
 		trace_binder_transaction_received(t);
 		binder_stat_br(proc, thread, cmd);
 		binder_debug(BINDER_DEBUG_TRANSACTION,

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

Posted by Hui Peng 11 hours ago

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@vger.kernel.org
Reviewed-by: Carlos Llamas <cmllamas@google.com>
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
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;