[PATCH] binder: fix null deref of proc->context

[PATCH] binder: fix null deref of proc->context

[Todd Kjos]

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>
---
 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

Re: [PATCH] binder: fix null deref of proc->context

[Christian Brauner]

On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>

Thanks, looks good to me!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Christian

> ---
>  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
> 

Re: [PATCH] binder: fix null deref of proc->context

[Todd Kjos]

On Mon, Jun 22, 2020 at 1:09 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>

Forgot to include stable. The issue was introduced in 5.6, so fix needed in 5.7.
Cc: stable@vger.kernel.org # 5.7


>
>
> Thanks, looks good to me!
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>
> Christian
>
> > ---
> >  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
> >

Re: [PATCH] binder: fix null deref of proc->context

[Todd Kjos]

On Mon, Jun 22, 2020 at 1:18 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Mon, Jun 22, 2020 at 1:09 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>
>
> Forgot to include stable. The issue was introduced in 5.6, so fix needed in 5.7.
> Cc: stable@vger.kernel.org # 5.7

Turns out the patch with the issue was also backported to 5.4.y, so
the fix is needed there too.

>
>
> >
> >
> > Thanks, looks good to me!
> > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > Christian
> >
> > > ---
> > >  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
> > >

Re: [PATCH] binder: fix null deref of proc->context

[Greg Kroah-Hartman]

On Mon, Jun 22, 2020 at 01:59:04PM -0700, Todd Kjos wrote:
> On Mon, Jun 22, 2020 at 1:18 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > On Mon, Jun 22, 2020 at 1:09 PM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > >
> > > On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>
> >
> > Forgot to include stable. The issue was introduced in 5.6, so fix needed in 5.7.
> > Cc: stable@vger.kernel.org # 5.7
> 
> Turns out the patch with the issue was also backported to 5.4.y, so
> the fix is needed there too.

With the fixes tag in there and cc: stable, it will get to the proper
trees no matter how far back it was backported :)

thanks,

greg k-h

Re: [PATCH] binder: fix null deref of proc->context

[Dan Carpenter]

On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>
> ---
>  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);
> +	}

Where is device allocated?

It looks to me like they are allocated in init_binder_device().  So why
are calling misc_deregister?  And it looks like the kfree(proc->context->name);
is wrong as well because that's from the
"device_names = kstrdup(binder_devices_param, GFP_KERNEL);" in
binder_init().

To be honest, I'm a bit confused why we're not doing this in module_exit().

regards,
dan carpenter

Re: [PATCH] binder: fix null deref of proc->context

[Christian Brauner]

On Tue, Jun 23, 2020 at 11:50:21AM +0300, Dan Carpenter wrote:
> On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>
> > ---
> >  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);
> > +	}
> 
> Where is device allocated?
> 
> It looks to me like they are allocated in init_binder_device().  So why
> are calling misc_deregister?  And it looks like the kfree(proc->context->name);
> is wrong as well because that's from the
> "device_names = kstrdup(binder_devices_param, GFP_KERNEL);" in
> binder_init().

This whole codepath is only hit for binderfs binder devices which are
allocated in binderfs.c. Legacy binder devices allocated through the
misc device layer always have a reference count > 0. When they are
opened there refcount is inced by one and when they are put is deced by
1 since their refcount starts from 1 they're always guaranteed to be at
1. For binderfs binder devices they start with a refcount of 1 as well
and it is incremented when they are opened too but there's another dec on
their refcount when the binderfs instance is shutting down or when they
are deleted through inode eviction. If someone is still keeping the
binderfs device alive through proc-> then the final put can't happen
during inode eviction but must happen in binder_free_proc() instead.

> 
> To be honest, I'm a bit confused why we're not doing this in module_exit().

Because there's no module_exit() for binder and with binderfs. Their
either "y" or "n". If you want to do this work then this can become a
little messy since you'd need to mess with userspace when you suddenly
invalidate all their references through a forced module unload.

Christian

Re: [PATCH] binder: fix null deref of proc->context

[Dan Carpenter]

On Tue, Jun 23, 2020 at 11:04:04AM +0200, Christian Brauner wrote:
> On Tue, Jun 23, 2020 at 11:50:21AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 22, 2020 at 01:07:15PM -0700, Todd Kjos 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>
> > > ---
> > >  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);
> > > +	}
> > 
> > Where is device allocated?
> > 
> > It looks to me like they are allocated in init_binder_device().  So why
> > are calling misc_deregister?  And it looks like the kfree(proc->context->name);
> > is wrong as well because that's from the
> > "device_names = kstrdup(binder_devices_param, GFP_KERNEL);" in
> > binder_init().
> 
> This whole codepath is only hit for binderfs binder devices which are
> allocated in binderfs.c.

Ah.  I see that now.  Thanks!

regards,
dan carpenter

Re: [PATCH] binder: fix null deref of proc->context

[Joel Fernandes]

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.
>