Messages in this thread | | | Date | Thu, 14 Feb 2019 14:45:40 -0500 | From | Joel Fernandes <> | Subject | Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function |
| |
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
| |