binder: make sure fd closes complete

PATCH In review Todd Kjos 5 participants 8 messages Aug 30, 2021 – Sep 14, 2021

Subsystems (2)

Stable branch Android drivers

lore

Share & tools

lore

Todd Kjos

author

2021-08-30 19:52 UTC #0 lore raw

PATCH 1/1 binder: make sure fd closes complete

During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.

Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.

Signed-off-by Todd Kjos <[email protected]>

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

drivers/android/binder.c +17 -6

index bcec598b89f2..c2823f0d588f 100644
@@ -1852,6 +1852,7 @@ static void binder_deferred_fd_close(int fd)
 }
 
 static void binder_transaction_buffer_release(struct binder_proc *proc,
+					      struct binder_thread *thread,
 					      struct binder_buffer *buffer,
 					      binder_size_t failed_at,
 					      bool is_failure)

@@ -2011,8 +2012,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 						&proc->alloc, &fd, buffer,
 						offset, sizeof(fd));
 				WARN_ON(err);
-				if (!err)
+				if (!err) {
 					binder_deferred_fd_close(fd);
+					/*
+					 * Need to make sure the thread goes
+					 * back to userspace to complete the
+					 * deferred close
+					 */
+					if (thread)
+						thread->looper_need_return = true;
+				}
 			}
 		} break;
 		default:

@@ -3105,7 +3114,7 @@ static void binder_transaction(struct binder_proc *proc,
 err_copy_data_failed:
 	binder_free_txn_fixups(t);
 	trace_binder_transaction_failed_buffer_release(t->buffer);
-	binder_transaction_buffer_release(target_proc, t->buffer,
+	binder_transaction_buffer_release(target_proc, NULL, t->buffer,
 					  buffer_offset, true);
 	if (target_node)
 		binder_dec_node_tmpref(target_node);

@@ -3184,7 +3193,9 @@ static void binder_transaction(struct binder_proc *proc,
  * Cleanup buffer and free it.
  */
 static void
-binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
+binder_free_buf(struct binder_proc *proc,
+		struct binder_thread *thread,
+		struct binder_buffer *buffer)
 {
 	binder_inner_proc_lock(proc);
 	if (buffer->transaction) {

@@ -3212,7 +3223,7 @@ binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
 		binder_node_inner_unlock(buf_node);
 	}
 	trace_binder_transaction_buffer_release(buffer);
-	binder_transaction_buffer_release(proc, buffer, 0, false);
+	binder_transaction_buffer_release(proc, thread, buffer, 0, false);
 	binder_alloc_free_buf(&proc->alloc, buffer);
 }

@@ -3414,7 +3425,7 @@ static int binder_thread_write(struct binder_proc *proc,
 				     proc->pid, thread->pid, (u64)data_ptr,
 				     buffer->debug_id,
 				     buffer->transaction ? "active" : "finished");
-			binder_free_buf(proc, buffer);
+			binder_free_buf(proc, thread, buffer);
 			break;
 		}

@@ -4107,7 +4118,7 @@ static int binder_thread_read(struct binder_proc *proc,
 			buffer->transaction = NULL;
 			binder_cleanup_transaction(t, "fd fixups failed",
 						   BR_FAILED_REPLY);
-			binder_free_buf(proc, buffer);
+			binder_free_buf(proc, thread, buffer);
 			binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
 				     "%d:%d %stransaction %d fd fixups failed %d/%d, line %d\n",
 				     proc->pid, thread->pid,

Signature · 1 line

2.33.0.259.gc128427fd7-goog

Christian Brauner

Acked-by on v1 1/1

2021-08-31 07:24 UTC #1 lore raw

Quoted 11 lines · Todd Kjos

During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>
---

Looks good. Thanks!

Acked-by Christian Brauner <[email protected]>

Martijn Coenen

Reviewed-by on v1 1/1

2021-08-31 07:24 UTC #2 lore raw

In reply to Todd Kjos

Quoted 11 lines · Todd Kjos' via kernel-team

During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>

Reviewed-by Martijn Coenen <[email protected]>

Quoted 85 lines

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

drivers/android/binder.c +17 -7

index bcec598b89f2..c2823f0d588f 100644
@@ -1852,6 +1852,7 @@ static void binder_deferred_fd_close(int fd)
 }
 
 static void binder_transaction_buffer_release(struct binder_proc *proc,
+                                             struct binder_thread *thread,
                                              struct binder_buffer *buffer,
                                              binder_size_t failed_at,
                                              bool is_failure)

@@ -2011,8 +2012,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
                                                &proc->alloc, &fd, buffer,
                                                offset, sizeof(fd));
                                WARN_ON(err);
-                               if (!err)
+                               if (!err) {
                                        binder_deferred_fd_close(fd);
+                                       /*
+                                        * Need to make sure the thread goes
+                                        * back to userspace to complete the
+                                        * deferred close
+                                        */
+                                       if (thread)
+                                               thread->looper_need_return = true;
+                               }
                        }
                } break;
                default:

@@ -3105,7 +3114,7 @@ static void binder_transaction(struct binder_proc *proc,
 err_copy_data_failed:
        binder_free_txn_fixups(t);
        trace_binder_transaction_failed_buffer_release(t->buffer);
-       binder_transaction_buffer_release(target_proc, t->buffer,
+       binder_transaction_buffer_release(target_proc, NULL, t->buffer,
                                          buffer_offset, true);
        if (target_node)
                binder_dec_node_tmpref(target_node);

@@ -3184,7 +3193,9 @@ static void binder_transaction(struct binder_proc *proc,
  * Cleanup buffer and free it.
  */
 static void
-binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
+binder_free_buf(struct binder_proc *proc,
+               struct binder_thread *thread,
+               struct binder_buffer *buffer)
 {
        binder_inner_proc_lock(proc);
        if (buffer->transaction) {

@@ -3212,7 +3223,7 @@ binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
                binder_node_inner_unlock(buf_node);
        }
        trace_binder_transaction_buffer_release(buffer);
-       binder_transaction_buffer_release(proc, buffer, 0, false);
+       binder_transaction_buffer_release(proc, thread, buffer, 0, false);
        binder_alloc_free_buf(&proc->alloc, buffer);
 }

@@ -3414,7 +3425,7 @@ static int binder_thread_write(struct binder_proc *proc,
                                     proc->pid, thread->pid, (u64)data_ptr,
                                     buffer->debug_id,
                                     buffer->transaction ? "active" : "finished");
-                       binder_free_buf(proc, buffer);
+                       binder_free_buf(proc, thread, buffer);
                        break;
                }

@@ -4107,7 +4118,7 @@ static int binder_thread_read(struct binder_proc *proc,
                        buffer->transaction = NULL;
                        binder_cleanup_transaction(t, "fd fixups failed",
                                                   BR_FAILED_REPLY);
-                       binder_free_buf(proc, buffer);
+                       binder_free_buf(proc, thread, buffer);
                        binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
                                     "%d:%d %stransaction %d fd fixups failed %d/%d, line %d\n",
                                     proc->pid, thread->pid,
--

2.33.0.259.gc128427fd7-goog

Todd Kjos

author

2021-09-02 15:35 UTC #3 lore raw

Quoted 10 lines · Martijn Coenen

Quoted 7 lines · Todd Kjos' via kernel-team
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.

Quoted 5 lines

Quoted 4 lines
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>

Please also add to stable releases 5.4 and later.

Quoted 86 lines

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

drivers/android/binder.c +17 -7

index bcec598b89f2..c2823f0d588f 100644
@@ -1852,6 +1852,7 @@ static void binder_deferred_fd_close(int fd)
 }
 
 static void binder_transaction_buffer_release(struct binder_proc *proc,
+                                             struct binder_thread *thread,
                                              struct binder_buffer *buffer,
                                              binder_size_t failed_at,
                                              bool is_failure)

@@ -2011,8 +2012,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
                                                &proc->alloc, &fd, buffer,
                                                offset, sizeof(fd));
                                WARN_ON(err);
-                               if (!err)
+                               if (!err) {
                                        binder_deferred_fd_close(fd);
+                                       /*
+                                        * Need to make sure the thread goes
+                                        * back to userspace to complete the
+                                        * deferred close
+                                        */
+                                       if (thread)
+                                               thread->looper_need_return = true;
+                               }
                        }
                } break;
                default:

@@ -3105,7 +3114,7 @@ static void binder_transaction(struct binder_proc *proc,
 err_copy_data_failed:
        binder_free_txn_fixups(t);
        trace_binder_transaction_failed_buffer_release(t->buffer);
-       binder_transaction_buffer_release(target_proc, t->buffer,
+       binder_transaction_buffer_release(target_proc, NULL, t->buffer,
                                          buffer_offset, true);
        if (target_node)
                binder_dec_node_tmpref(target_node);

@@ -3184,7 +3193,9 @@ static void binder_transaction(struct binder_proc *proc,
  * Cleanup buffer and free it.
  */
 static void
-binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
+binder_free_buf(struct binder_proc *proc,
+               struct binder_thread *thread,
+               struct binder_buffer *buffer)
 {
        binder_inner_proc_lock(proc);
        if (buffer->transaction) {

@@ -3212,7 +3223,7 @@ binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
                binder_node_inner_unlock(buf_node);
        }
        trace_binder_transaction_buffer_release(buffer);
-       binder_transaction_buffer_release(proc, buffer, 0, false);
+       binder_transaction_buffer_release(proc, thread, buffer, 0, false);
        binder_alloc_free_buf(&proc->alloc, buffer);
 }

@@ -3414,7 +3425,7 @@ static int binder_thread_write(struct binder_proc *proc,
                                     proc->pid, thread->pid, (u64)data_ptr,
                                     buffer->debug_id,
                                     buffer->transaction ? "active" : "finished");
-                       binder_free_buf(proc, buffer);
+                       binder_free_buf(proc, thread, buffer);
                        break;
                }

@@ -4107,7 +4118,7 @@ static int binder_thread_read(struct binder_proc *proc,
                        buffer->transaction = NULL;
                        binder_cleanup_transaction(t, "fd fixups failed",
                                                   BR_FAILED_REPLY);
-                       binder_free_buf(proc, buffer);
+                       binder_free_buf(proc, thread, buffer);
                        binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
                                     "%d:%d %stransaction %d fd fixups failed %d/%d, line %d\n",
                                     proc->pid, thread->pid,
--

2.33.0.259.gc128427fd7-goog

Greg KH

maintainer

2021-09-02 16:30 UTC #4 lore raw

Quoted 11 lines · Todd Kjos

Quoted 10 lines · Martijn Coenen
Quoted 7 lines · Todd Kjos' via kernel-team
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.

Quoted 7 lines

Quoted 5 lines
Quoted 4 lines
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Please also add to stable releases 5.4 and later.

I'll try to remember to tag this as-such after 5.15-rc1 is out and I can apply it to my tree. But in the future, it's best if you add the cc:
stable to the patch yourself so I don't have to do it "by hand" after the fact.

thanks,

greg k-h

Dan Carpenter

2021-09-03 08:06 UTC #5 lore raw

In reply to Todd Kjos

Quoted 11 lines · Todd Kjos

Quoted 10 lines · Martijn Coenen
Quoted 7 lines · Todd Kjos' via kernel-team
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.

Quoted 7 lines

Quoted 5 lines
Quoted 4 lines
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Please also add to stable releases 5.4 and later.

It would be better if this had a fixes tag so we knew which is the first buggy commit.

There was a long Project Zero article about the Bad Binder exploit because commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when thread exits.") was marked as # 4.14 but it didn't have a Fixes tag and the actual buggy commit was in 4.9.

regards,
dan carpenter

Todd Kjos

author

2021-09-03 19:38 UTC #6 lore raw

Quoted 21 lines · Dan Carpenter

Quoted 11 lines · Todd Kjos
Quoted 10 lines · Martijn Coenen
Quoted 7 lines · Todd Kjos' via kernel-team
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Please also add to stable releases 5.4 and later.

Quoted 7 lines

It would be better if this had a fixes tag so we knew which is the first buggy commit.
There was a long Project Zero article about the Bad Binder exploit because commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when thread exits.") was marked as # 4.14 but it didn't have a Fixes tag and the actual buggy commit was in 4.9.

Good point Dan. I should have included a Fixes tag. Here is the tag (issue introduced in 4.20):

Fixes 80cd795630d6 ("binder: fix use-after-free due to ksys_close()

during fdget()")

Greg- would you like me to send a v2 with the Fixes tag and CC'ing stable appropriately?

Quoted 7 lines

regards,
dan carpenter
--
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].

Greg KH

maintainer

2021-09-14 07:01 UTC #7 lore raw

Quoted 30 lines · Todd Kjos

Quoted 21 lines · Dan Carpenter
Quoted 11 lines · Todd Kjos
Quoted 10 lines · Martijn Coenen
Quoted 7 lines · Todd Kjos' via kernel-team
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes.
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling.
Signed-off-by Todd Kjos <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Please also add to stable releases 5.4 and later.
Quoted 7 lines
It would be better if this had a fixes tag so we knew which is the first buggy commit.
There was a long Project Zero article about the Bad Binder exploit because commit f5cb779ba163 ("ANDROID: binder: remove waitqueue when thread exits.") was marked as # 4.14 but it didn't have a Fixes tag and the actual buggy commit was in 4.9.

Quoted 8 lines

Good point Dan. I should have included a Fixes tag. Here is the tag (issue introduced in 4.20):
Fixes 80cd795630d6 ("binder: fix use-after-free due to ksys_close()
during fdget()")
Greg- would you like me to send a v2 with the Fixes tag and CC'ing stable appropriately?

I've added it to the commit when I added it to my tree, no need to resend.

thanks,

greg k-h