linux-kernel - Re: [PATCH] android: binder: use VM

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]

Date:   Thu, 1 Sep 2016 12:59:37 -0700
From:   Arve Hjønnevåg <arve@...roid.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Ganesh Mahendran <opensource.ganesh@...il.com>,
        Riley Andrews <riandrews@...roid.com>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

On Thu, Sep 1, 2016 at 12:02 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>> VM_IOREMAP is used to access hardware through a mechanism called
>> I/O mapped memory. Android binder is a IPC machanism which will
>> not access I/O memory.
>>
>> Also VM_IOREMAP has alignment requiement which may not needed in
>> binder.
>> ----
>> __get_vm_area_node()
>> {
>> ...
>>     if (flags & VM_IOREMAP)
>>         align = 1ul << clamp_t(int, fls_long(size),
>>            PAGE_SHIFT, IOREMAP_MAX_ORDER);
>> ...
>> }
>> ----
>>
>> This patch use VM_ALLOC to get vm area.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@...il.com>
>> ---
>>  drivers/android/binder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 16288e7..3511d5c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>>               goto err_already_mapped;
>>       }
>>
>> -     area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>> +     area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>>       if (area == NULL) {
>>               ret = -ENOMEM;
>>               failure_string = "get_vm_area";
>
> What change have you noticed with this patch?  Have you tested it?
> Found that previously reserved iomemory is now free for binder to use
> where it wasn't?  What kind of change does the system now run as because
> of this?
>
> And are you _sure_ the alignment requirement isn't needed for binder?
> Have you verified this with the userspace binder library?
>
> This is messy, tricky, stuff, I'm loath to change it without loads of
> testing having happened...
>
> thanks,
>
> greg k-h

There is no alignment requirement on this area unless
cache_is_vipt_aliasing returns true. In that case the area needs to be
aligned with vma->vm_start which is done manually in the code right
after this allocation. If there are no other side effects of changing
this flag this change should be safe, but please run all the tests at
https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/
to test it.

-- 
Arve Hjønnevåg