Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753684AbdHXUaJ (ORCPT ); Thu, 24 Aug 2017 16:30:09 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:44974 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751168AbdHXUaI (ORCPT ); Thu, 24 Aug 2017 16:30:08 -0400 Date: Thu, 24 Aug 2017 23:29:44 +0300 From: Dan Carpenter To: Sherry Yang Cc: linux-kernel@vger.kernel.org, "open list:ANDROID DRIVERS" , Greg Kroah-Hartman , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , maco@google.com, tkjos@google.com Subject: Re: [PATCH v2 3/5] android: binder: Move buffer out of area shared with user space Message-ID: <20170824202943.5euttnd7hlbdpyj2@mwanda> References: <20170823154643.2613-1-sherryy@android.com> <20170823154643.2613-4-sherryy@android.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170823154643.2613-4-sherryy@android.com> User-Agent: NeoMutt/20170113 (1.7.2) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1633 Lines: 46 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