linux-kernel - Re: [PATCH v3 3/6] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMP5XgeyOWTv=gs+msHCrPfZE5bTQ2rsNk2+EYzbRY6MbsHPAw@mail.gmail.com>
Date:   Wed, 30 Aug 2017 13:04:31 -0700
From:   Arve Hjønnevåg <arve@...roid.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Sherry Yang <sherryy@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Riley Andrews <riandrews@...roid.com>,
        Martijn Coenen <maco@...gle.com>, Todd Kjos <tkjos@...gle.com>
Subject: Re: [PATCH v3 3/6] android: binder: Move buffer out of area shared
 with user space

On Wed, Aug 30, 2017 at 2:29 AM, Dan Carpenter <dan.carpenter@...cle.com> 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 <sherryy@...roid.com>
>> ---
>
> 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@...gle.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