Binder bug fixes

PATCH In review Arve Hjønnevåg 4 participants 11 messages Jan 21, 2012 – Feb 2, 2012 Staging

lore

Share & tools

lore

Arve Hjønnevåg

author

2012-01-21 03:56 UTC #0 lore raw

cover Binder bug fixes

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

Arve Hjønnevåg

author

2012-01-21 03:57 UTC #1 lore raw

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

- 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 <[email protected]>

---
 drivers/staging/android/binder.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

drivers/staging/android/binder.c +11 -1

index 7491801..846b429 100644
@@ -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);

Signature · 1 line

1.7.7.3

Dan Carpenter

2012-01-21 08:21 UTC #2 lore raw

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

Quoted 6 lines · 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.

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

Greg KH

2012-01-31 18:58 UTC #3 lore raw

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

Quoted 11 lines · Dan Carpenter

Quoted 6 lines · 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.
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

Arve Hjønnevåg

author

2012-01-31 23:20 UTC #4 lore raw

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

2012/1/31 Greg KH <[email protected]>:

Quoted 9 lines · Dan Carpenter

Quoted 8 lines · Arve Hjønnevåg
Quoted 6 lines
- 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.

Quoted 7 lines

Quoted 3 lines
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.

Signature · 1 line

Arve Hjønnevåg

Dan Carpenter

2012-02-01 06:51 UTC #5 lore raw

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

Quoted 14 lines · Arve Hjønnevåg

2012/1/31 Greg KH <[email protected]>:
Quoted 13 lines · Dan Carpenter
Quoted 11 lines · Arve Hjønnevåg
Quoted 6 lines
- 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.

Quoted 7 lines

Quoted 3 lines
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

Arve Hjønnevåg

author

2012-02-01 22:29 UTC #6 lore raw

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

2012/1/31 Dan Carpenter <[email protected]>:

Quoted 19 lines · Arve Hjønnevåg

Quoted 18 lines
2012/1/31 Greg KH <[email protected]>:
Quoted 16 lines · Dan Carpenter
Quoted 11 lines · Arve Hjønnevåg
Quoted 6 lines
- 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.

Quoted 9 lines

Quoted 3 lines
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.

Signature · 1 line

Arve Hjønnevåg

Greg KH

maintainer

2012-02-01 22:47 UTC #7 lore raw

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

Quoted 36 lines · Arve Hjønnevåg

2012/1/31 Dan Carpenter <[email protected]>:
Quoted 19 lines · Arve Hjønnevåg
Quoted 18 lines
2012/1/31 Greg KH <[email protected]>:
Quoted 16 lines · Dan Carpenter
Quoted 11 lines · Arve Hjønnevåg
Quoted 6 lines
- 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.
Quoted 9 lines
Quoted 3 lines
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

Quoted 9 lines

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

Arve Hjønnevåg

author

2012-02-01 23:29 UTC #8 lore raw

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

Signed-off-by Arve Hjønnevåg <[email protected]>

---
 drivers/staging/android/binder.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

drivers/staging/android/binder.c +11 -1

index 48cf27c..f0b7e66 100644
@@ -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);

Signature · 1 line

1.7.7.3

Dan Carpenter

Acked-by

2012-02-02 06:26 UTC #9 lore raw

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

In reply to Greg KH

Quoted 1 line · Greg KH

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

Looks good.

Acked-by Dan Carpenter <[email protected]>

regards,
dan carpenter

Arve Hjønnevåg

author

2012-01-21 03:57 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/android/binder.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/staging/android/binder.c +0 -1

index 846b429..f0b7e66 100644
@@ -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)

Signature · 1 line

1.7.7.3