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 <tk...@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
