From: Christian Brauner <christian.brauner@ubuntu.com> To: Dan Carpenter <dan.carpenter@oracle.com> Cc: Todd Kjos <tkjos@google.com>, gregkh@linuxfoundation.org, christian@brauner.io, arve@android.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, maco@google.com, joel@joelfernandes.org, kernel-team@android.com Subject: Re: [PATCH] binder: fix null deref of proc->context Date: Tue, 23 Jun 2020 11:04:04 +0200 [thread overview] Message-ID: <20200623090404.xwuhdec6c7p4lnd2@wittgenstein> (raw) In-Reply-To: <20200623085021.GG4151@kadam> 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
WARNING: multiple messages have this Message-ID (diff)
From: Christian Brauner <christian.brauner@ubuntu.com> To: Dan Carpenter <dan.carpenter@oracle.com> Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, arve@android.com, maco@google.com, joel@joelfernandes.org, kernel-team@android.com, christian@brauner.io, Todd Kjos <tkjos@google.com> Subject: Re: [PATCH] binder: fix null deref of proc->context Date: Tue, 23 Jun 2020 11:04:04 +0200 [thread overview] Message-ID: <20200623090404.xwuhdec6c7p4lnd2@wittgenstein> (raw) In-Reply-To: <20200623085021.GG4151@kadam> 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 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-06-23 9:04 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 [this message] 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 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=20200623090404.xwuhdec6c7p4lnd2@wittgenstein \ --to=christian.brauner@ubuntu.com \ --cc=arve@android.com \ --cc=christian@brauner.io \ --cc=dan.carpenter@oracle.com \ --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.