Re: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()
From: Serban Constantinescu
Date: Thu Dec 05 2013 - 13:44:58 EST
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@xxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/