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

[Arve HjÃnnevÃg]

On Wed, Aug 30, 2017 at 2:29 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>
> 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@xxxxxxxxxx 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