All of lore.kernel.org
 help / color / mirror / Atom feed
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

  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: link
Be 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.