Re: [PATCH v2 3/5] android: binder: Move buffer out of area shared with user space

[Dan Carpenter]

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