| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 05 Dec 2013 18:44:55 +0000
From: Serban Constantinescu <Serban.Constantinescu@....com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: "arve@...roid.com" <arve@...roid.com>,
"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"john.stultz@...aro.org" <john.stultz@...aro.org>,
"ccross@...roid.com" <ccross@...roid.com>,
Dave Butcher <Dave.Butcher@....com>,
"irogers@...gle.com" <irogers@...gle.com>,
"romlem@...roid.com" <romlem@...roid.com>
Subject: Re: [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@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists