[PATCH 0/2] Binder bug fixes

[PATCH 0/2] Binder bug fixes

[Arve Hjønnevåg]

Fix some problems reported in https://lkml.org/lkml/2011/12/22/311

[PATCH 1/2] Staging: android: binder: Add some error checks

[Arve Hjønnevåg]

- Add a mutex to protect against two processes mmapping the
  same binder_proc.
- After locking mmap_sem, check that the vma we want to access
  (still) points to the same mm_struct.
- Use proc->tsk instead of current to get the files struct since
  this is where we get the rlimit from.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 7491801..846b429 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -38,6 +38,7 @@
 
 static DEFINE_MUTEX(binder_lock);
 static DEFINE_MUTEX(binder_deferred_lock);
+static DEFINE_MUTEX(binder_mmap_lock);
 
 static HLIST_HEAD(binder_procs);
 static HLIST_HEAD(binder_deferred_list);
@@ -632,6 +633,11 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
 	if (mm) {
 		down_write(&mm->mmap_sem);
 		vma = proc->vma;
+		if (vma && mm != vma->vm_mm) {
+			pr_err("binder: %d: vma mm and task mm mismatch\n",
+				proc->pid);
+			vma = NULL;
+		}
 	}
 
 	if (allocate == 0)
@@ -2803,6 +2809,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
 
+	mutex_lock(&binder_mmap_lock);
 	if (proc->buffer) {
 		ret = -EBUSY;
 		failure_string = "already mapped";
@@ -2817,6 +2824,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	proc->buffer = area->addr;
 	proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
+	mutex_unlock(&binder_mmap_lock);
 
 #ifdef CONFIG_CPU_CACHE_VIPT
 	if (cache_is_vipt_aliasing()) {
@@ -2849,7 +2857,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	binder_insert_free_buffer(proc, buffer);
 	proc->free_async_space = proc->buffer_size / 2;
 	barrier();
-	proc->files = get_files_struct(current);
+	proc->files = get_files_struct(proc->tsk);
 	proc->vma = vma;
 
 	/*printk(KERN_INFO "binder_mmap: %d %lx-%lx maps %p\n",
@@ -2860,10 +2868,12 @@ err_alloc_small_buf_failed:
 	kfree(proc->pages);
 	proc->pages = NULL;
 err_alloc_pages_failed:
+	mutex_lock(&binder_mmap_lock);
 	vfree(proc->buffer);
 	proc->buffer = NULL;
 err_get_vm_area_failed:
 err_already_mapped:
+	mutex_unlock(&binder_mmap_lock);
 err_bad_arg:
 	printk(KERN_ERR "binder_mmap: %d %lx-%lx %s failed %d\n",
 	       proc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
-- 
1.7.7.3

[PATCH 2/2] Staging: android: binder: Don't call dump_stack in binder_vma_open

[Arve Hjønnevåg]

If user-space partially unmaps the driver, binder_vma_open
would dump the kernel stack. This is not a kernel bug however
and will be treated as if the whole area was unmapped once
binder_vma_close gets called.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 846b429..f0b7e66 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2765,7 +2765,6 @@ static void binder_vma_open(struct vm_area_struct *vma)
 		     proc->pid, vma->vm_start, vma->vm_end,
 		     (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
 		     (unsigned long)pgprot_val(vma->vm_page_prot));
-	dump_stack();
 }
 
 static void binder_vma_close(struct vm_area_struct *vma)
-- 
1.7.7.3

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote:
> - Add a mutex to protect against two processes mmapping the
>   same binder_proc.
> - After locking mmap_sem, check that the vma we want to access
>   (still) points to the same mm_struct.
> - Use proc->tsk instead of current to get the files struct since
>   this is where we get the rlimit from.

This doesn't seem related to the locking change at all.  Probably
this patch should be split into three patches, one bugfix per
patch, unless they are very closely related.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Greg KH]

On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote:
> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote:
> > - Add a mutex to protect against two processes mmapping the
> >   same binder_proc.
> > - After locking mmap_sem, check that the vma we want to access
> >   (still) points to the same mm_struct.
> > - Use proc->tsk instead of current to get the files struct since
> >   this is where we get the rlimit from.
> 
> This doesn't seem related to the locking change at all.  Probably
> this patch should be split into three patches, one bugfix per
> patch, unless they are very closely related.

I agree.  Arve, is this all fixing one problem, or multiple ones?  If
multiple ones, we need this split up into multiple patches.

thanks,

greg k-h

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Arve Hjønnevåg]

2012/1/31 Greg KH <greg@kroah.com>:
> On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote:
>> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote:
>> > - Add a mutex to protect against two processes mmapping the
>> >   same binder_proc.
>> > - After locking mmap_sem, check that the vma we want to access
>> >   (still) points to the same mm_struct.
>> > - Use proc->tsk instead of current to get the files struct since
>> >   this is where we get the rlimit from.
>>
>> This doesn't seem related to the locking change at all.  Probably
>> this patch should be split into three patches, one bugfix per
>> patch, unless they are very closely related.
>
> I agree.  Arve, is this all fixing one problem, or multiple ones?  If
> multiple ones, we need this split up into multiple patches.
>

That depend on your point of view. It fixes crashes if you use the
same binder file pointer from multiple processes. It seemed excessive
to have three patches for this.

-- 
Arve Hjønnevåg

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hjønnevåg wrote:
> 2012/1/31 Greg KH <greg@kroah.com>:
> > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote:
> >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote:
> >> > - Add a mutex to protect against two processes mmapping the
> >> >   same binder_proc.
> >> > - After locking mmap_sem, check that the vma we want to access
> >> >   (still) points to the same mm_struct.
> >> > - Use proc->tsk instead of current to get the files struct since
> >> >   this is where we get the rlimit from.
> >>
> >> This doesn't seem related to the locking change at all.  Probably
> >> this patch should be split into three patches, one bugfix per
> >> patch, unless they are very closely related.
> >
> > I agree.  Arve, is this all fixing one problem, or multiple ones?  If
> > multiple ones, we need this split up into multiple patches.
> >
> 
> That depend on your point of view. It fixes crashes if you use the
> same binder file pointer from multiple processes. It seemed excessive
> to have three patches for this.

It would have helped you to write a better changelog.  The subject
says "[patch] android: grab bag of random fixes" and the the
description matches that.  You have no idea how annoyed I get at
grab bag patches.

Also don't ignore review comments.  I review a lot of staging
patches and I'm not an expert on every driver so my review comments
are often wrong.  I don't get upset when people tell me that.
Review is part of the process.  Everybody does it.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Arve Hjønnevåg]

2012/1/31 Dan Carpenter <dan.carpenter@oracle.com>:
> On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hjønnevåg wrote:
>> 2012/1/31 Greg KH <greg@kroah.com>:
>> > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote:
>> >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote:
>> >> > - Add a mutex to protect against two processes mmapping the
>> >> >   same binder_proc.
>> >> > - After locking mmap_sem, check that the vma we want to access
>> >> >   (still) points to the same mm_struct.
>> >> > - Use proc->tsk instead of current to get the files struct since
>> >> >   this is where we get the rlimit from.
>> >>
>> >> This doesn't seem related to the locking change at all.  Probably
>> >> this patch should be split into three patches, one bugfix per
>> >> patch, unless they are very closely related.
>> >
>> > I agree.  Arve, is this all fixing one problem, or multiple ones?  If
>> > multiple ones, we need this split up into multiple patches.
>> >
>>
>> That depend on your point of view. It fixes crashes if you use the
>> same binder file pointer from multiple processes. It seemed excessive
>> to have three patches for this.
>
> It would have helped you to write a better changelog.  The subject
> says "[patch] android: grab bag of random fixes" and the the
> description matches that.  You have no idea how annoyed I get at
> grab bag patches.
>

Would the following be a better change description (or do you still
want three patches):

Staging: android: binder: Fix crashes when sharing a binder file
between processes

Opening the binder driver and sharing the file returned with
other processes (e.g. by calling fork) can crash the kernel.
Prevent these crashes with the following changes:
- Add a mutex to protect against two processes mmapping the
  same binder_proc.
- After locking mmap_sem, check that the vma we want to access
  (still) points to the same mm_struct.
- Use proc->tsk instead of current to get the files struct since
  this is where we get the rlimit from.



-- 
Arve Hjønnevåg

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Greg KH]

On Wed, Feb 01, 2012 at 02:29:36PM -0800, Arve Hjønnevåg wrote:
> 2012/1/31 Dan Carpenter <dan.carpenter@oracle.com>:
> > On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hjønnevåg wrote:
> >> 2012/1/31 Greg KH <greg@kroah.com>:
> >> > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote:
> >> >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote:
> >> >> > - Add a mutex to protect against two processes mmapping the
> >> >> >   same binder_proc.
> >> >> > - After locking mmap_sem, check that the vma we want to access
> >> >> >   (still) points to the same mm_struct.
> >> >> > - Use proc->tsk instead of current to get the files struct since
> >> >> >   this is where we get the rlimit from.
> >> >>
> >> >> This doesn't seem related to the locking change at all.  Probably
> >> >> this patch should be split into three patches, one bugfix per
> >> >> patch, unless they are very closely related.
> >> >
> >> > I agree.  Arve, is this all fixing one problem, or multiple ones?  If
> >> > multiple ones, we need this split up into multiple patches.
> >> >
> >>
> >> That depend on your point of view. It fixes crashes if you use the
> >> same binder file pointer from multiple processes. It seemed excessive
> >> to have three patches for this.
> >
> > It would have helped you to write a better changelog.  The subject
> > says "[patch] android: grab bag of random fixes" and the the
> > description matches that.  You have no idea how annoyed I get at
> > grab bag patches.
> >
> 
> Would the following be a better change description (or do you still
> want three patches):
> 
> Staging: android: binder: Fix crashes when sharing a binder file
> between processes
> 
> Opening the binder driver and sharing the file returned with
> other processes (e.g. by calling fork) can crash the kernel.
> Prevent these crashes with the following changes:
> - Add a mutex to protect against two processes mmapping the
>   same binder_proc.
> - After locking mmap_sem, check that the vma we want to access
>   (still) points to the same mm_struct.
> - Use proc->tsk instead of current to get the files struct since
>   this is where we get the rlimit from.

That looks good to me, as one patch, Dan?

greg k-h

[PATCH] Staging: android: binder: Fix crashes when sharing a binder file between processes

[Arve Hjønnevåg]

Opening the binder driver and sharing the file returned with
other processes (e.g. by calling fork) can crash the kernel.
Prevent these crashes with the following changes:
- Add a mutex to protect against two processes mmapping the
  same binder_proc.
- After locking mmap_sem, check that the vma we want to access
  (still) points to the same mm_struct.
- Use proc->tsk instead of current to get the files struct since
  this is where we get the rlimit from.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 48cf27c..f0b7e66 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -38,6 +38,7 @@
 
 static DEFINE_MUTEX(binder_lock);
 static DEFINE_MUTEX(binder_deferred_lock);
+static DEFINE_MUTEX(binder_mmap_lock);
 
 static HLIST_HEAD(binder_procs);
 static HLIST_HEAD(binder_deferred_list);
@@ -632,6 +633,11 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
 	if (mm) {
 		down_write(&mm->mmap_sem);
 		vma = proc->vma;
+		if (vma && mm != vma->vm_mm) {
+			pr_err("binder: %d: vma mm and task mm mismatch\n",
+				proc->pid);
+			vma = NULL;
+		}
 	}
 
 	if (allocate == 0)
@@ -2802,6 +2808,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
 
+	mutex_lock(&binder_mmap_lock);
 	if (proc->buffer) {
 		ret = -EBUSY;
 		failure_string = "already mapped";
@@ -2816,6 +2823,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	proc->buffer = area->addr;
 	proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
+	mutex_unlock(&binder_mmap_lock);
 
 #ifdef CONFIG_CPU_CACHE_VIPT
 	if (cache_is_vipt_aliasing()) {
@@ -2848,7 +2856,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	binder_insert_free_buffer(proc, buffer);
 	proc->free_async_space = proc->buffer_size / 2;
 	barrier();
-	proc->files = get_files_struct(current);
+	proc->files = get_files_struct(proc->tsk);
 	proc->vma = vma;
 
 	/*printk(KERN_INFO "binder_mmap: %d %lx-%lx maps %p\n",
@@ -2859,10 +2867,12 @@ err_alloc_small_buf_failed:
 	kfree(proc->pages);
 	proc->pages = NULL;
 err_alloc_pages_failed:
+	mutex_lock(&binder_mmap_lock);
 	vfree(proc->buffer);
 	proc->buffer = NULL;
 err_get_vm_area_failed:
 err_already_mapped:
+	mutex_unlock(&binder_mmap_lock);
 err_bad_arg:
 	printk(KERN_ERR "binder_mmap: %d %lx-%lx %s failed %d\n",
 	       proc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
-- 
1.7.7.3

Re: [PATCH 1/2] Staging: android: binder: Add some error checks

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

On Wed, Feb 01, 2012 at 02:47:08PM -0800, Greg KH wrote:
> That looks good to me, as one patch, Dan?

Looks good.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]