Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy function
On Tue, Jan 29, 2019 at 04:32:09PM -0800, Todd Kjos wrote:
> On Tue, Jan 29, 2019 at 12:12 AM Dan Carpenter
> wrote:
> >
> > On Mon, Jan 28, 2019 at 04:49:28PM -0800, Todd Kjos wrote:
> > > +/**
> > > + * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
> > > + * @alloc: binder_alloc for this proc
> > > + * @buffer: binder buffer to be accessed
> > > + * @buffer_offset: offset into @buffer data
> > > + * @from: userspace pointer to source buffer
> > > + * @bytes: bytes to copy
> > > + *
> > > + * Copy bytes from source userspace to target buffer.
> > > + *
> > > + * Return: bytes remaining to be copied
> > > + */
> > > +unsigned long
> > > +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
> > > + struct binder_buffer *buffer,
> > > + binder_size_t buffer_offset,
> > > + const void __user *from,
> > > + size_t bytes)
> > > +{
> > > + if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> > > + return bytes;
> > > +
> > > + while (bytes) {
> > > + unsigned long size;
> > > + unsigned long ret;
> > > + struct page *page;
> > > + pgoff_t pgoff;
> > > + void *kptr;
> > > +
> > > + page = binder_alloc_get_page(alloc, buffer,
> > > + buffer_offset, );
> > > + size = min(bytes, (size_t)(PAGE_SIZE - pgoff));
> >
> > This code has so much more casting than necessary. To me casting says
> > that we haven't got the types correct or something. I've just pulled
> > this function out as an example really, but none of the casts here are
> > required... This could just be:
> >
> > size = min(bytes, PAGE_SIZE - pgoff);
>
> Dan,
>
> Thanks for the feedback.
>
> The one above: "size = min(bytes, PAGE_SIZE - pgoff);", results in
> 32-bit compile warnings:
>
> ./include/linux/kernel.h:846:29: warning: comparison of distinct
> pointer types lacks a cast
>(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> [...]
> drivers/android/binder_alloc.c:1157:10: note: in expansion of macro ‘min’
>size = min(bytes, PAGE_SIZE - pgoff);
>
> so, I took your suggestion:
>
> size = min_t(size_t, bytes, PAGE_SIZE - pgoff);
>
> >
> > Btw, if you really need to do a cast inside a min() (you don't in this
> > cast) then use min_t().
> >
> > > + kptr = (void *)((uintptr_t)kmap(page) + pgoff);
> >
> > This would be a lot cleaner as:
> >
> > kptr = kmap(page) + pgoff;
>
> It definitely is cleaner, but the clean version is doing arithmetic on
> a void pointer which is generally not considered good practice (but is
> supported by gcc). Is this acceptable in the kernel? I don't see any
> statement on it in Documentation/process/coding-style.rst.
Yeah, it's a GCCism but to me it seems so nice. We do it all the time.
We'd like to get Clang to compile the kernel, but we've done very little
work to make that happen. Removing variable length arrays was really
done for other reasons...
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy function
On Tue, Jan 29, 2019 at 12:12 AM Dan Carpenter wrote:
>
> On Mon, Jan 28, 2019 at 04:49:28PM -0800, Todd Kjos wrote:
> > +/**
> > + * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
> > + * @alloc: binder_alloc for this proc
> > + * @buffer: binder buffer to be accessed
> > + * @buffer_offset: offset into @buffer data
> > + * @from: userspace pointer to source buffer
> > + * @bytes: bytes to copy
> > + *
> > + * Copy bytes from source userspace to target buffer.
> > + *
> > + * Return: bytes remaining to be copied
> > + */
> > +unsigned long
> > +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
> > + struct binder_buffer *buffer,
> > + binder_size_t buffer_offset,
> > + const void __user *from,
> > + size_t bytes)
> > +{
> > + if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> > + return bytes;
> > +
> > + while (bytes) {
> > + unsigned long size;
> > + unsigned long ret;
> > + struct page *page;
> > + pgoff_t pgoff;
> > + void *kptr;
> > +
> > + page = binder_alloc_get_page(alloc, buffer,
> > + buffer_offset, );
> > + size = min(bytes, (size_t)(PAGE_SIZE - pgoff));
>
> This code has so much more casting than necessary. To me casting says
> that we haven't got the types correct or something. I've just pulled
> this function out as an example really, but none of the casts here are
> required... This could just be:
>
> size = min(bytes, PAGE_SIZE - pgoff);
Dan,
Thanks for the feedback.
The one above: "size = min(bytes, PAGE_SIZE - pgoff);", results in
32-bit compile warnings:
./include/linux/kernel.h:846:29: warning: comparison of distinct
pointer types lacks a cast
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
[...]
drivers/android/binder_alloc.c:1157:10: note: in expansion of macro ‘min’
size = min(bytes, PAGE_SIZE - pgoff);
so, I took your suggestion:
size = min_t(size_t, bytes, PAGE_SIZE - pgoff);
>
> Btw, if you really need to do a cast inside a min() (you don't in this
> cast) then use min_t().
>
> > + kptr = (void *)((uintptr_t)kmap(page) + pgoff);
>
> This would be a lot cleaner as:
>
> kptr = kmap(page) + pgoff;
It definitely is cleaner, but the clean version is doing arithmetic on
a void pointer which is generally not considered good practice (but is
supported by gcc). Is this acceptable in the kernel? I don't see any
statement on it in Documentation/process/coding-style.rst.
If so, I agree a bunch of these casts can go as you suggest.
-Todd
[...]
>
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy function
On Mon, Jan 28, 2019 at 04:49:28PM -0800, Todd Kjos wrote:
> +/**
> + * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
> + * @alloc: binder_alloc for this proc
> + * @buffer: binder buffer to be accessed
> + * @buffer_offset: offset into @buffer data
> + * @from: userspace pointer to source buffer
> + * @bytes: bytes to copy
> + *
> + * Copy bytes from source userspace to target buffer.
> + *
> + * Return: bytes remaining to be copied
> + */
> +unsigned long
> +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
> + struct binder_buffer *buffer,
> + binder_size_t buffer_offset,
> + const void __user *from,
> + size_t bytes)
> +{
> + if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> + return bytes;
> +
> + while (bytes) {
> + unsigned long size;
> + unsigned long ret;
> + struct page *page;
> + pgoff_t pgoff;
> + void *kptr;
> +
> + page = binder_alloc_get_page(alloc, buffer,
> + buffer_offset, );
> + size = min(bytes, (size_t)(PAGE_SIZE - pgoff));
This code has so much more casting than necessary. To me casting says
that we haven't got the types correct or something. I've just pulled
this function out as an example really, but none of the casts here are
required... This could just be:
size = min(bytes, PAGE_SIZE - pgoff);
Btw, if you really need to do a cast inside a min() (you don't in this
cast) then use min_t().
> + kptr = (void *)((uintptr_t)kmap(page) + pgoff);
This would be a lot cleaner as:
kptr = kmap(page) + pgoff;
> + ret = copy_from_user(kptr, (const void __user *)(uintptr_t)from,
> + size);
Remove the cast:
ret = copy_from_user(kptr, from, size);
> + kunmap(page);
> + if (ret)
> + return bytes - size + ret;
> + bytes -= size;
> + from = (void __user *)(uintptr_t)from + size;
Remove the cast:
from += size;
> + buffer_offset += size;
> + }
> + return 0;
> +}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/7] binder: create userspace-to-binder-buffer copy function
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
---
drivers/android/binder.c | 29 +++--
drivers/android/binder_alloc.c | 114 +
drivers/android/binder_alloc.h | 8 +++
3 files changed, 144 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(
+ _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(
+ _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(
+ _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..255fa71911e5e 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -29,6 +29,8 @@
#include
#include
#include
+#include
+#include
#include "binder_alloc.h"
#include "binder_trace.h"
@@ -1053,3 +1055,115 @@ 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
+ *
