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]

Message-ID: <20160901190245.GA21183@kroah.com>
Date:   Thu, 1 Sep 2016 21:02:45 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Ganesh Mahendran <opensource.ganesh@...il.com>
Cc:     arve@...roid.com, riandrews@...roid.com,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

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