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(-)
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
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 <[email protected]>
> ---
Looks good. Thanks!
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
<[email protected]> 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 <[email protected]>
Reviewed-by: Martijn Coenen <[email protected]>
> ---
> 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
>
>
On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote:
> On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen <[email protected]> wrote:
> >
> > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
> > <[email protected]> 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 <[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
On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen <[email protected]> wrote:
>
> On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
> <[email protected]> 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 <[email protected]>
> Reviewed-by: Martijn Coenen <[email protected]>
Please also add to stable releases 5.4 and later.
>
> > ---
> > 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
> >
> >
On Thu, Sep 02, 2021 at 08:35:35AM -0700, Todd Kjos wrote:
> On Tue, Aug 31, 2021 at 12:24 AM Martijn Coenen <[email protected]> wrote:
> >
> > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
> > <[email protected]> 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 <[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
On Fri, Sep 3, 2021 at 1:06 AM Dan Carpenter <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
> > > <[email protected]> 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 <[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.
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 [email protected].
>
On Fri, Sep 03, 2021 at 12:38:26PM -0700, Todd Kjos wrote:
> On Fri, Sep 3, 2021 at 1:06 AM Dan Carpenter <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
> > > > <[email protected]> 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 <[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.
>
> 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