Re: [PATCH] binder: make sure fd closes complete
On Fri, Sep 03, 2021 at 12:38:26PM -0700, Todd Kjos wrote: > On Fri, Sep 3, 2021 at 1:06 AM Dan Carpenter wrote: > > > > On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote: > > > On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen wrote: > > > > > > > > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team > > > > wrote: > > > > > > > > > > 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 > > > > Reviewed-by: Martijn Coenen > > > > > > 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. > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Fri, Sep 3, 2021 at 1:06 AM Dan Carpenter wrote: > > On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote: > > On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen wrote: > > > > > > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team > > > wrote: > > > > > > > > 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 > > > Reviewed-by: Martijn Coenen > > > > 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. 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? > > regards, > dan carpenter > > -- > To unsubscribe from this group and stop receiving emails from it, send an > email to kernel-team+unsubscr...@android.com. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote: > On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen wrote: > > > > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team > > wrote: > > > > > > 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 > > Reviewed-by: Martijn Coenen > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote: > On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen wrote: > > > > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team > > wrote: > > > > > > 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 > > Reviewed-by: Martijn Coenen > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team wrote: > > 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 Reviewed-by: Martijn Coenen > --- > drivers/android/binder.c | 23 +-- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index bcec598b89f2..c2823f0d588f 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -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 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Mon, Aug 30, 2021 at 12:51:46PM -0700, Todd Kjos wrote: > 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 > --- Looks good. Thanks! Acked-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel