Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751385AbdH3UEd (ORCPT ); Wed, 30 Aug 2017 16:04:33 -0400 Received: from mail-pg0-f50.google.com ([74.125.83.50]:36136 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751330AbdH3UEc (ORCPT ); Wed, 30 Aug 2017 16:04:32 -0400 MIME-Version: 1.0 In-Reply-To: <20170830092904.b43bmlveo5g4c6rd@mwanda> References: <20170830004702.120371-1-sherryy@android.com> <20170830004702.120371-4-sherryy@android.com> <20170830092904.b43bmlveo5g4c6rd@mwanda> From: =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= Date: Wed, 30 Aug 2017 13:04:31 -0700 Message-ID: Subject: Re: [PATCH v3 3/6] android: binder: Move buffer out of area shared with user space To: Dan Carpenter Cc: Sherry Yang , LKML , "open list:ANDROID DRIVERS" , Greg Kroah-Hartman , Riley Andrews , Martijn Coenen , Todd Kjos Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v7UK4bSN002083 Content-Length: 2399 Lines: 52 On Wed, Aug 30, 2017 at 2:29 AM, Dan Carpenter wrote: > On Tue, Aug 29, 2017 at 05:46:59PM -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. >> >> Also move alloc->buffers initialization from mmap to init since it's >> now used even when mmap failed or was not called. >> >> Signed-off-by: Sherry Yang >> --- > > The difference between v2 and v3 is that we've shifted some > initialization around to fix the crashing bug that kbuild found. You > should not that difference here under the --- cut off. > >> drivers/android/binder_alloc.c | 146 +++++++++++++++++++------------- >> drivers/android/binder_alloc.h | 2 +- >> drivers/android/binder_alloc_selftest.c | 11 ++- >> 3 files changed, 91 insertions(+), 68 deletions(-) > > But really we still need to have some answers or discussion about the > questions that Greg and I raised. Greg asked if the other Android devs > had Acked this. Please ping Arve to Ack this. > tkjos@google.com replied and ack'ed v2. The changes have been reviewed on android-review.googlesource.com. Do you want and ack or review tag included in the patchset or do you want separate ack emails on each patchset (or on each patch)? > I was curious about the security impact or why we were writing this > patch 3/6. It seems we are fixing an information disclosure bug. Or is > it something worse than that? Or have I misunderstood entirely. > > We probably original put the buffers in userspace for accounting reasons > so we could kill programs that used too much RAM. This patch doesn't > create a problem with that hopefully? We're just moving the metadata to > kernel space? > The buffer headers have never been used by user-space. They are readable by user-space because the content after the header has to be readable from user-space (and only whole pages can be mapped). It was simpler to have the header in the same shared region as well. At the time this code was written the content of kernel pointers where not considered secret and available elsewhere anyway (e.g. kernel log, /proc/kallsyms). -- Arve Hjønnevåg