Re: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()

[Serban Constantinescu <Serban.Constantinescu@arm.com>]

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