From: Joel Fernandes <joelaf@google.com> To: Todd Kjos <tkjos@google.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Christian Brauner <christian@brauner.io>, Arve Hjonnevag <arve@android.com>, "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>, LKML <linux-kernel@vger.kernel.org>, Martijn Coenen <maco@google.com>, "Joel Fernandes (Google)" <joel@joelfernandes.org>, "Cc: Android Kernel" <kernel-team@android.com> Subject: Re: [PATCH] binder: fix null deref of proc->context Date: Tue, 23 Jun 2020 09:54:27 -0400 [thread overview] Message-ID: <CAJWu+op-5EjRiaL7b-TcXCruqheET9aB_ZJK8CGsfmuxSNQWqA@mail.gmail.com> (raw) In-Reply-To: <20200622200715.114382-1-tkjos@google.com> On Mon, Jun 22, 2020 at 4:07 PM 'Todd Kjos' via kernel-team <kernel-team@android.com> wrote: > > The binder driver makes the assumption proc->context pointer is invariant after > initialization (as documented in the kerneldoc header for struct proc). > However, in commit f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") > proc->context is set to NULL during binder_deferred_release(). > > Another proc was in the middle of setting up a transaction to the dying > process and crashed on a NULL pointer deref on "context" which is a local > set to &proc->context: > > new_ref->data.desc = (node == context->binder_context_mgr_node) ? 0 : 1; > > Here's the stack: > > [ 5237.855435] Call trace: > [ 5237.855441] binder_get_ref_for_node_olocked+0x100/0x2ec > [ 5237.855446] binder_inc_ref_for_node+0x140/0x280 > [ 5237.855451] binder_translate_binder+0x1d0/0x388 > [ 5237.855456] binder_transaction+0x2228/0x3730 > [ 5237.855461] binder_thread_write+0x640/0x25bc > [ 5237.855466] binder_ioctl_write_read+0xb0/0x464 > [ 5237.855471] binder_ioctl+0x30c/0x96c > [ 5237.855477] do_vfs_ioctl+0x3e0/0x700 > [ 5237.855482] __arm64_sys_ioctl+0x78/0xa4 > [ 5237.855488] el0_svc_common+0xb4/0x194 > [ 5237.855493] el0_svc_handler+0x74/0x98 > [ 5237.855497] el0_svc+0x8/0xc > > The fix is to move the kfree of the binder_device to binder_free_proc() > so the binder_device is freed when we know there are no references > remaining on the binder_proc. > > Fixes: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") > Signed-off-by: Todd Kjos <tkjos@google.com> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> Thanks, - Joel > --- > drivers/android/binder.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index e47c8a4c83db..f50c5f182bb5 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -4686,8 +4686,15 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc) > > static void binder_free_proc(struct binder_proc *proc) > { > + struct binder_device *device; > + > BUG_ON(!list_empty(&proc->todo)); > BUG_ON(!list_empty(&proc->delivered_death)); > + device = container_of(proc->context, struct binder_device, context); > + if (refcount_dec_and_test(&device->ref)) { > + kfree(proc->context->name); > + kfree(device); > + } > binder_alloc_deferred_release(&proc->alloc); > put_task_struct(proc->tsk); > binder_stats_deleted(BINDER_STAT_PROC); > @@ -5406,7 +5413,6 @@ static int binder_node_release(struct binder_node *node, int refs) > static void binder_deferred_release(struct binder_proc *proc) > { > struct binder_context *context = proc->context; > - struct binder_device *device; > struct rb_node *n; > int threads, nodes, incoming_refs, outgoing_refs, active_transactions; > > @@ -5423,12 +5429,6 @@ static void binder_deferred_release(struct binder_proc *proc) > context->binder_context_mgr_node = NULL; > } > mutex_unlock(&context->context_mgr_node_lock); > - device = container_of(proc->context, struct binder_device, context); > - if (refcount_dec_and_test(&device->ref)) { > - kfree(context->name); > - kfree(device); > - } > - proc->context = NULL; > binder_inner_proc_lock(proc); > /* > * Make sure proc stays alive after we > -- > 2.27.0.111.gc72c7da667-goog > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
WARNING: multiple messages have this Message-ID (diff)
From: Joel Fernandes <joelaf@google.com> To: Todd Kjos <tkjos@google.com> Cc: "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, LKML <linux-kernel@vger.kernel.org>, Arve Hjonnevag <arve@android.com>, Martijn Coenen <maco@google.com>, "Joel Fernandes \(Google\)" <joel@joelfernandes.org>, "Cc: Android Kernel" <kernel-team@android.com>, Christian Brauner <christian@brauner.io> Subject: Re: [PATCH] binder: fix null deref of proc->context Date: Tue, 23 Jun 2020 09:54:27 -0400 [thread overview] Message-ID: <CAJWu+op-5EjRiaL7b-TcXCruqheET9aB_ZJK8CGsfmuxSNQWqA@mail.gmail.com> (raw) In-Reply-To: <20200622200715.114382-1-tkjos@google.com> On Mon, Jun 22, 2020 at 4:07 PM 'Todd Kjos' via kernel-team <kernel-team@android.com> wrote: > > The binder driver makes the assumption proc->context pointer is invariant after > initialization (as documented in the kerneldoc header for struct proc). > However, in commit f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") > proc->context is set to NULL during binder_deferred_release(). > > Another proc was in the middle of setting up a transaction to the dying > process and crashed on a NULL pointer deref on "context" which is a local > set to &proc->context: > > new_ref->data.desc = (node == context->binder_context_mgr_node) ? 0 : 1; > > Here's the stack: > > [ 5237.855435] Call trace: > [ 5237.855441] binder_get_ref_for_node_olocked+0x100/0x2ec > [ 5237.855446] binder_inc_ref_for_node+0x140/0x280 > [ 5237.855451] binder_translate_binder+0x1d0/0x388 > [ 5237.855456] binder_transaction+0x2228/0x3730 > [ 5237.855461] binder_thread_write+0x640/0x25bc > [ 5237.855466] binder_ioctl_write_read+0xb0/0x464 > [ 5237.855471] binder_ioctl+0x30c/0x96c > [ 5237.855477] do_vfs_ioctl+0x3e0/0x700 > [ 5237.855482] __arm64_sys_ioctl+0x78/0xa4 > [ 5237.855488] el0_svc_common+0xb4/0x194 > [ 5237.855493] el0_svc_handler+0x74/0x98 > [ 5237.855497] el0_svc+0x8/0xc > > The fix is to move the kfree of the binder_device to binder_free_proc() > so the binder_device is freed when we know there are no references > remaining on the binder_proc. > > Fixes: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") > Signed-off-by: Todd Kjos <tkjos@google.com> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> Thanks, - Joel > --- > drivers/android/binder.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index e47c8a4c83db..f50c5f182bb5 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -4686,8 +4686,15 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc) > > static void binder_free_proc(struct binder_proc *proc) > { > + struct binder_device *device; > + > BUG_ON(!list_empty(&proc->todo)); > BUG_ON(!list_empty(&proc->delivered_death)); > + device = container_of(proc->context, struct binder_device, context); > + if (refcount_dec_and_test(&device->ref)) { > + kfree(proc->context->name); > + kfree(device); > + } > binder_alloc_deferred_release(&proc->alloc); > put_task_struct(proc->tsk); > binder_stats_deleted(BINDER_STAT_PROC); > @@ -5406,7 +5413,6 @@ static int binder_node_release(struct binder_node *node, int refs) > static void binder_deferred_release(struct binder_proc *proc) > { > struct binder_context *context = proc->context; > - struct binder_device *device; > struct rb_node *n; > int threads, nodes, incoming_refs, outgoing_refs, active_transactions; > > @@ -5423,12 +5429,6 @@ static void binder_deferred_release(struct binder_proc *proc) > context->binder_context_mgr_node = NULL; > } > mutex_unlock(&context->context_mgr_node_lock); > - device = container_of(proc->context, struct binder_device, context); > - if (refcount_dec_and_test(&device->ref)) { > - kfree(context->name); > - kfree(device); > - } > - proc->context = NULL; > binder_inner_proc_lock(proc); > /* > * Make sure proc stays alive after we > -- > 2.27.0.111.gc72c7da667-goog > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-06-23 13:54 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-22 20:07 [PATCH] binder: fix null deref of proc->context Todd Kjos 2020-06-22 20:07 ` Todd Kjos 2020-06-22 20:09 ` Christian Brauner 2020-06-22 20:09 ` Christian Brauner 2020-06-22 20:18 ` Todd Kjos 2020-06-22 20:18 ` Todd Kjos 2020-06-22 20:59 ` Todd Kjos 2020-06-22 20:59 ` Todd Kjos 2020-06-23 5:22 ` Greg Kroah-Hartman 2020-06-23 5:22 ` Greg Kroah-Hartman 2020-06-23 8:50 ` Dan Carpenter 2020-06-23 8:50 ` Dan Carpenter 2020-06-23 9:04 ` Christian Brauner 2020-06-23 9:04 ` Christian Brauner 2020-06-23 10:13 ` Dan Carpenter 2020-06-23 10:13 ` Dan Carpenter 2020-06-23 13:54 ` Joel Fernandes [this message] 2020-06-23 13:54 ` Joel Fernandes
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJWu+op-5EjRiaL7b-TcXCruqheET9aB_ZJK8CGsfmuxSNQWqA@mail.gmail.com \ --to=joelaf@google.com \ --cc=arve@android.com \ --cc=christian@brauner.io \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=joel@joelfernandes.org \ --cc=kernel-team@android.com \ --cc=linux-kernel@vger.kernel.org \ --cc=maco@google.com \ --cc=tkjos@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.