Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 14 Feb 2019 14:45:40 -0500
From:   Joel Fernandes <joelaf@google.com>
To:     Todd Kjos <tkjos@android.com>
Cc:     tkjos@google.com, gregkh@linuxfoundation.org, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, joel@joelfernandes.org, kernel-team@android.com
Subject: Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy
 function
Message-ID: <20190214194540.GA185075@google.com>
References: <20190208183520.30886-1-tkjos@google.com>
 <20190208183520.30886-2-tkjos@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190208183520.30886-2-tkjos@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Todd,

One quick question:

On Fri, Feb 08, 2019 at 10:35:14AM -0800, Todd Kjos wrote:
> The binder driver uses a vm_area to map the per-process
> binder buffer space. For 32-bit android devices, this is
> now taking too much vmalloc space. This patch removes
> the use of vm_area when copying the transaction data
> from the sender to the buffer space. Instead of using
> copy_from_user() for multi-page copies, it now uses
> binder_alloc_copy_user_to_buffer() which uses kmap()
> and kunmap() to map each page, and uses copy_from_user()
> for copying to that page.
> 
> Signed-off-by: Todd Kjos <tkjos@google.com>
> ---
> v2: remove casts as suggested by Dan Carpenter
> 
>  drivers/android/binder.c       |  29 +++++++--
>  drivers/android/binder_alloc.c | 113 +++++++++++++++++++++++++++++++++
>  drivers/android/binder_alloc.h |   8 +++
>  3 files changed, 143 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 5f6ef5e63b91e..ab0b3eec363bc 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3078,8 +3078,12 @@ static void binder_transaction(struct binder_proc *proc,
>  				      ALIGN(tr->data_size, sizeof(void *)));
>  	offp = off_start;
>  
> -	if (copy_from_user(t->buffer->data, (const void __user *)(uintptr_t)
> -			   tr->data.ptr.buffer, tr->data_size)) {
> +	if (binder_alloc_copy_user_to_buffer(
> +				&target_proc->alloc,
> +				t->buffer, 0,
> +				(const void __user *)
> +					(uintptr_t)tr->data.ptr.buffer,
> +				tr->data_size)) {
>  		binder_user_error("%d:%d got transaction with invalid data ptr\n",
>  				proc->pid, thread->pid);
>  		return_error = BR_FAILED_REPLY;
> @@ -3087,8 +3091,13 @@ static void binder_transaction(struct binder_proc *proc,
>  		return_error_line = __LINE__;
>  		goto err_copy_data_failed;
>  	}
> -	if (copy_from_user(offp, (const void __user *)(uintptr_t)
> -			   tr->data.ptr.offsets, tr->offsets_size)) {
> +	if (binder_alloc_copy_user_to_buffer(
> +				&target_proc->alloc,
> +				t->buffer,
> +				ALIGN(tr->data_size, sizeof(void *)),
> +				(const void __user *)
> +					(uintptr_t)tr->data.ptr.offsets,
> +				tr->offsets_size)) {
>  		binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
>  				proc->pid, thread->pid);
>  		return_error = BR_FAILED_REPLY;
> @@ -3217,6 +3226,8 @@ static void binder_transaction(struct binder_proc *proc,
>  			struct binder_buffer_object *bp =
>  				to_binder_buffer_object(hdr);
>  			size_t buf_left = sg_buf_end - sg_bufp;
> +			binder_size_t sg_buf_offset = (uintptr_t)sg_bufp -
> +				(uintptr_t)t->buffer->data;
>  
>  			if (bp->length > buf_left) {
>  				binder_user_error("%d:%d got transaction with too large buffer\n",
> @@ -3226,9 +3237,13 @@ static void binder_transaction(struct binder_proc *proc,
>  				return_error_line = __LINE__;
>  				goto err_bad_offset;
>  			}
> -			if (copy_from_user(sg_bufp,
> -					   (const void __user *)(uintptr_t)
> -					   bp->buffer, bp->length)) {
> +			if (binder_alloc_copy_user_to_buffer(
> +						&target_proc->alloc,
> +						t->buffer,
> +						sg_buf_offset,
> +						(const void __user *)
> +							(uintptr_t)bp->buffer,
> +						bp->length)) {
>  				binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
>  						  proc->pid, thread->pid);
>  				return_error_param = -EFAULT;
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 022cd80e80cc3..94c0d85c4e75b 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -29,6 +29,8 @@
>  #include <linux/list_lru.h>
>  #include <linux/ratelimit.h>
>  #include <asm/cacheflush.h>
> +#include <linux/uaccess.h>
> +#include <linux/highmem.h>
>  #include "binder_alloc.h"
>  #include "binder_trace.h"
>  
> @@ -1053,3 +1055,114 @@ int binder_alloc_shrinker_init(void)
>  	}
>  	return ret;
>  }
> +
> +/**
> + * check_buffer() - verify that buffer/offset is safe to access
> + * @alloc: binder_alloc for this proc
> + * @buffer: binder buffer to be accessed
> + * @offset: offset into @buffer data
> + * @bytes: bytes to access from offset
> + *
> + * Check that the @offset/@bytes are within the size of the given
> + * @buffer and that the buffer is currently active and not freeable.
> + * Offsets must also be multiples of sizeof(u32). The kernel is

In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
alignment instead of u32?

thanks,

 - Joel