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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170824202943.5euttnd7hlbdpyj2@mwanda>
Date:   Thu, 24 Aug 2017 23:29:44 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Sherry Yang <sherryy@...roid.com>
Cc:     linux-kernel@...r.kernel.org,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Riley Andrews <riandrews@...roid.com>, maco@...gle.com,
        tkjos@...gle.com
Subject: Re: [PATCH v2 3/5] android: binder: Move buffer out of area shared
 with user space

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