[android-kernel] [PATCH 1/2] binder: Fix memory corruption via page aliasing

[Christopher Lais]

binder_deferred_release was not unmapping the page from the buffer
before freeing it, causing memory corruption. This only happened
when page(s) had not been freed by binder_update_page_range, which
properly unmaps the pages.

To reproduce, create a program which opens, mmaps, munmaps, then closes
the binder very quickly. This should leave a page allocated when the
binder is released. Different alloc_page / area->addr combinations
with shared area->addr need to happen for the problem to have an
effect, because otherwise the buffer will just have the same pages
mapped to it as before.

PAGE_POISONING will greatly increase your chances of noticing any
changes. Adding debug to show the non-contiguously mapped page
addresses (phys_to_virt(page_to_phys(page->pages[i]))) on allocation
and free in binder_deferred_release may help with verifying.

Change-Id: I6941bf212881b8bf846bdfda43d3609c7ae4892e
---
drivers/staging/android/binder.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index cd53c64..1d06967 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3026,11 +3026,13 @@ static void binder_deferred_release(struct binder_proc *proc)
int i;
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
if (proc->pages[i]) {
+ void *page_addr = proc->buffer + i * PAGE_SIZE;
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
"binder_release: %d: "
"page %d at %p not freed\n",
proc->pid, i,
- proc->buffer + i * PAGE_SIZE);
+ page_addr);
+ unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
__free_page(proc->pages[i]);
page_count++;
}
--
1.7.1

--
unsubscribe: android-kerne...@googlegroups.com
website: http://groups.google.com/group/android-kernel

[Christopher Lais]

The binder proc buffers were being allocated with VM_IOREMAP, possibly
to ensure properly aligned allocations. This had the side effect of
allowing get_vm_area to reallocate mapped IO memory instead of
returning a valid usable area. The VIPT aliasing check was never
triggered before, but did not update the offset after the check, so the
cache color was still different between the mappings.

VM_IOREMAP has been replaced with VM_MAP to avoid remapping IO memory, and
the user page offset assignment has been moved to after the vipt aliasing
adjustment, so the userspace mmapped area and proc->buffer have the same
cache color.

To reproduce, create a program which opens, then mmaps many binders.
This will exhaust memory, but before it does it will map proc buffers
to previously-mapped IO ranges. You may need to add debugging that
prints the address returned by get_vm_area to directly see the effect,
but most devices are not happy when IO is remapped.

You may be able to use a program that also munmaps and closes the binders,
to avoid memory exhaustion, if your kernel allocates new pages each time.

Change-Id: Ibc55f6885efa39f3b4722927bdf8ac91476d754d
---
drivers/staging/android/binder.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 1d06967..53e7515 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2792,14 +2792,13 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
goto err_already_mapped;
}

- area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
+ area = get_vm_area(vma->vm_end - vma->vm_start, VM_MAP);
if (area == NULL) {
ret = -ENOMEM;
failure_string = "get_vm_area";
goto err_get_vm_area_failed;
}
proc->buffer = area->addr;
- proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;

#ifdef CONFIG_CPU_CACHE_VIPT
if (cache_is_vipt_aliasing()) {
@@ -2809,6 +2808,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
}
}
#endif
+ proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
proc->pages = kzalloc(sizeof(proc->pages[0]) * ((vma->vm_end - vma->vm_start) / PAGE_SIZE), GFP_KERNEL);
if (proc->pages == NULL) {
ret = -ENOMEM;

[Christopher Lais]

Sadly, this patch does not appear to work. get_vm_area is still
returning virtual addresses that are already mapped to IO.

--
Christopher Lais

[Christopher Lais]

2010/5/1 Christopher Lais <chris+...@zenthought.org>:

> Sadly, this patch does not appear to work.  get_vm_area is still
> returning virtual addresses that are already mapped to IO.
>
> --
> Christopher Lais

This turned out to be a platform specific bug (msm7k trout /
sapphire); fixed in a different patch.

[Christopher Lais]

binder_deferred_release was not unmapping the page from the buffer
before freeing it, causing memory corruption. This only happened
when page(s) had not been freed by binder_update_page_range, which
properly unmaps the pages.

To reproduce, create a program which opens, mmaps, munmaps, then closes
the binder very quickly. This should leave a page allocated when the

binder is released. When binder_deferrred_release is called on the
close, the page will remain mapped to the address in the linear
proc->buffer, but that area may be returned by alloc_page with the
mapping to a different physical page in-tact.

PAGE_POISONING will greatly increase your chances of noticing any

problems. Adding debug to show the addresses of the allog_page'd pages
(page_address(page->pages[i])) and the proc->buffer on allocation in
binder_mmap and on free in binder_deferred_release may help with