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

[Hui Peng]

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@xxxxxxxxxxxxxxx
Reviewed-by: Carlos Llamas <cmllamas@xxxxxxxxxx>
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
---
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,