Re: [PATCH v2 3/7] staging: android: binder: fix binder interfacefor 64bit compat layer

From: Serban Constantinescu
Date: Wed Apr 10 2013 - 09:02:06 EST


On 10/04/13 00:48, Arve Hjønnevåg wrote:
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 5794cf6..a2cdd9e 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
...
@@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
}

int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, size_t size, size_t *consumed)

What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.

This change is related to the userspace handling of the struct binder_write_read. The userspace writes write_size and read_size as size_t values(size_t Parcel::dataSize(), size_t Parcel::dataCapacity()).

On return from a BINDER_WRITE_READ ioctl write_consumed and read_consumed are checked against positive values(these values will represent the difference between the start of the buffer cursor and the current buffer start - positive values since buffer cursor = buffer start ++).

However if there is any plan for these values to be handled as signed longs at some point we can change the patch such that we modify just the function prototype to:

int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, signed long size, signed long *consumed)

I will break this change into its own patch such that it is easier to grasp.


...
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index dbe81ce..8012921 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
*/
struct flat_binder_object {
/* 8 bytes for large_flat_header. */
- unsigned long type;
- unsigned long flags;
+ __u32 type;
+ __u32 flags;

/* 8 bytes of data. */
union {
void __user *binder; /* local object */
- signed long handle; /* remote object */
+ __s32 handle; /* remote object */

Why limit the handle to 32 bits when the pointer that it shares
storage with need to be 64 bit on 64 bit systems?

Here I have mirrored the type being passed in handle - a file descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type == BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside the kernel/userspace(as 32bit value on 64bit systems). However this change does not limit the extension of the API since we can read the value as 64bit - binder(on 64bit systems).

I can remove this change if you consider that is the better solution.

};

/* extra data associated with local object */
@@ -67,18 +67,18 @@ struct flat_binder_object {
*/

struct binder_write_read {
- signed long write_size; /* bytes to write */
- signed long write_consumed; /* bytes consumed by driver */
+ size_t write_size; /* bytes to write */
+ size_t write_consumed; /* bytes consumed by driver */
unsigned long write_buffer;
- signed long read_size; /* bytes to read */
- signed long read_consumed; /* bytes consumed by driver */
+ size_t read_size; /* bytes to read */
+ size_t read_consumed; /* bytes consumed by driver */

What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.

See above explanation for binder_thread_write() change, I will break this into its own patch.

unsigned long read_buffer;
};

/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;

How does user-space know if it should use 32 bit or 64 bit pointers.
Without this change, the BINDER_VERSION ioctl would only match when
the size of long matches.

The userspace can check the values returned by uname(). That will determine if the kernel is 32 or 64bit and depending on this select what binder structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels using protocol_version as 64bit signed long(that is old kernel versions with no 64bit support).

Leaving this value as signed long would mean that older versions of the binder(without 64bit support) will pass the check. Furthermore the protocol version will probably never exceed the values that could be represented on 32bit. It will also mean that BINDER_VERSION will have a different userspace/kernel handler for 64/32 systems.

Let me know what are your thoughts related to these changes,
Thanks for your feedback,
Serban

--
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/