lkml.org 
[lkml]   [2013]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()
On 04/12/13 23:17, Greg KH wrote:
> On Wed, Dec 04, 2013 at 06:09:34PM +0000, Serban Constantinescu wrote:
>> This patch adds binder_copy_to_user() to be used for copying binder
>> commands to user address space. This way we can abstract away the
>> copy_to_user() calls and add separate handling for the compat layer.
>>
>> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
>> ---
>> drivers/staging/android/binder.c | 39 ++++++++++++++++++++------------------
>> 1 file changed, 21 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
>> index 233889c..6fbb340 100644
>> --- a/drivers/staging/android/binder.c
>> +++ b/drivers/staging/android/binder.c
>> @@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread *thread)
>> (thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
>> }
>>
>> +static int binder_copy_to_user(uint32_t cmd, void *parcel,
>> + void __user **ptr, size_t size)
>> +{
>> + if (put_user(cmd, (uint32_t __user *)*ptr))
>> + return -EFAULT;
>> + *ptr += sizeof(uint32_t);
>> + if (copy_to_user(*ptr, parcel, size))
>> + return -EFAULT;
>> + *ptr += size;
>> + return 0;
>> +}
>
> I know what you are trying to do here, but ick, why not just use the
> structure involved in the copying out here? Or just copy the thing out
> in one "chunk", not two different calls, which should make this go
> faster, right?

Ick... agree. I do this split here for the compat handling added in the
next patches, where the cmd will have to be converted to a 32bit compat
cmd and the size, parcel ,passed here will change to a 32bit compat
size, parcel.

This patch makes more sense when looking at the following snippet:

*<snippet from patch 9/9>*

> +static int binder_copy_to_user(uint32_t cmd, void *parcel,
> + void __user **ptr, size_t size)
> +{
> + if (!is_compat_task()) {
> + if (put_user(cmd, (uint32_t __user *)*ptr))
> + return -EFAULT;
> + *ptr += sizeof(uint32_t);
> + if (copy_to_user(*ptr, parcel, size))
> + return -EFAULT;
> + *ptr += size;
> + return 0;
> + }
> + switch (cmd) {
> + case BR_INCREFS:
> + case BR_ACQUIRE:
> + case BR_RELEASE:
> + case BR_DECREFS: {
> + struct binder_ptr_cookie *fp;
> + struct compat_binder_ptr_cookie tmp;
> +
> + cmd = compat_change_size(cmd, sizeof(tmp));

Passing the cmd, and the structure to be copied to binder_copy_to_user()
allows me to add this extra handling here. Where, first, cmd is changed
to a compat cmd, and then copied to userspace - i.e:

I change

> cmd = BR_INCREFS = _IOR('r', 7, struct binder_ptr_cookie)

to

> cmd = COMPAT_BR_INCREFS = _IOR('r', 7, struct compat_binder_ptr_cookie)

where

> struct binder_ptr_cookie {
> void* ptr;
> void* cookie;
> };

and

> struct compat_binder_ptr_cookie {
> compat_uptr_t ptr;
> compat_uptr_t cookie;
> };

Thus BR_INCREFS will be different between 32bit userspace and 64bit kernel.

> + BUG_ON((cmd != COMPAT_BR_INCREFS) &&
> + (cmd != COMPAT_BR_ACQUIRE) &&
> + (cmd != COMPAT_BR_RELEASE) &&
> + (cmd != COMPAT_BR_DECREFS));
> +
> + fp = (struct binder_ptr_cookie *) parcel;
> + tmp.ptr = ptr_to_compat(fp->ptr);
> + tmp.cookie = ptr_to_compat(fp->cookie);
> + if (put_user(cmd, (uint32_t __user *)*ptr))
> + return -EFAULT;
> + *ptr += sizeof(uint32_t);
> + if (copy_to_user(*ptr, &tmp, sizeof(tmp)))
> + return -EFAULT;
> + *ptr += sizeof(tmp);

Also, since the size of the parcel will differ when copied to a compat
task (32bit userspace) I increment the buffer ptr here, rather then
doing this in binder_thread_read().

This way we can safely move to the next cmd, by incrementing the buffer
ptr accordingly whether 32 or 64bit structures are needed.

*</sippet from patch 9/9>*


>> +
>> static int binder_thread_read(struct binder_proc *proc,
>> struct binder_thread *thread,
>> void __user *buffer, size_t size,
>> @@ -2263,15 +2275,12 @@ retry:
>> node->has_weak_ref = 0;
>> }
>> if (cmd != BR_NOOP) {
>> - if (put_user(cmd, (uint32_t __user *)ptr))
>> - return -EFAULT;
>> - ptr += sizeof(uint32_t);
>> - if (put_user(node->ptr, (void * __user *)ptr))
>> - return -EFAULT;
>> - ptr += sizeof(void *);
>> - if (put_user(node->cookie, (void * __user *)ptr))
>> + struct binder_ptr_cookie tmp;
>> +
>> + tmp.ptr = node->ptr;
>> + tmp.cookie = node->cookie;
>> + if (binder_copy_to_user(cmd, &tmp, &ptr, sizeof(struct binder_ptr_cookie)))
>> return -EFAULT;
>> - ptr += sizeof(void *);
>
> Are you sure this is correct? You are now no longer incrementing ptr
> anymore, is that ok with the larger loop here?

Obscure, I should document this in the commit message! It is correct, I
increment the buffer in binder_copy_to_user() with a different size,
depending on whether the structures are copied to 32bit or 64bit
userspace. See above explanation for more detail.

>
>
>>
>> binder_stat_br(proc, thread, cmd);
>> binder_debug(BINDER_DEBUG_USER_REFS,
>> @@ -2306,12 +2315,10 @@ retry:
>> cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
>> else
>> cmd = BR_DEAD_BINDER;
>> - if (put_user(cmd, (uint32_t __user *)ptr))
>> - return -EFAULT;
>> - ptr += sizeof(uint32_t);
>> - if (put_user(death->cookie, (void * __user *)ptr))
>> +
>> + if (binder_copy_to_user(cmd, &death->cookie, &ptr, sizeof(void *)))
>> return -EFAULT;
>> - ptr += sizeof(void *);
>> +
>
> Same here, no more ptr incrementing.

See above.
>
>
>> binder_stat_br(proc, thread, cmd);
>> binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
>> "%d:%d %s %p\n",
>> @@ -2373,12 +2380,8 @@ retry:
>> ALIGN(t->buffer->data_size,
>> sizeof(void *));
>>
>> - if (put_user(cmd, (uint32_t __user *)ptr))
>> - return -EFAULT;
>> - ptr += sizeof(uint32_t);
>> - if (copy_to_user(ptr, &tr, sizeof(tr)))
>> + if (binder_copy_to_user(cmd, &tr, &ptr, sizeof(struct binder_transaction_data)))
>> return -EFAULT;
>> - ptr += sizeof(tr);
>
> And here, no more ptr incrementing.
See above.

Thanks for your feedback Greg,
Serban C



\
 
 \ /
  Last update: 2013-12-05 20:01    [W:1.639 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site