On Wed, Aug 23, 2017 at 08:46:41AM -0700, Sherry Yang wrote:
> Binder driver allocates buffer meta data in a region that is mapped
> in user space. These meta data contain pointers in the kernel.
>
> This patch allocates buffer meta data on the kernel heap that is
> not mapped in user space, and uses a pointer to refer to the data mapped.
>
This feels like it has a security impact, right? The original code is
an info leak?
> @@ -664,7 +679,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
>
> return 0;
>
> -err_alloc_small_buf_failed:
> +err_alloc_buf_struct_failed:
> kfree(alloc->pages);
> alloc->pages = NULL;
> err_alloc_pages_failed:
Not really really related to your patch, I was just looking at the
error handling here. It looks like this with your patch applied.
682 err_alloc_buf_struct_failed:
683 kfree(alloc->pages);
684 alloc->pages = NULL;
685 err_alloc_pages_failed:
686 mutex_lock(&binder_alloc_mmap_lock);
687 vfree(alloc->buffer);
The vfree() here is supposed to release the resources from get_vm_area().
Why do people not use free_vm_area() instead? It feels like we're
freeing "area->addr" but leaking "area" itself but perhaps I have
misunderstood something.
688 alloc->buffer = NULL;
689 err_get_vm_area_failed:
690 err_already_mapped:
691 mutex_unlock(&binder_alloc_mmap_lock);
692 pr_err("%s: %d %lx-%lx %s failed %d\n", __func__,
693 alloc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
694 return ret;
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
