[PATCH 0/2] staging: android: ashmem: add 32bit compat support

[PATCH 0/2] staging: android: ashmem: add 32bit compat support

[Serban Constantinescu]

Hi all,

This set of patches will add support for existing 32bit Android ashmem
syscalls on 64bit platforms. They also fix the ashmem pin/unpin interface
without affecting the 32bit ABI but imposing a new, correct one, for any
future 64bit Android userspace.

They have been successfully tested on 32bit platforms(VExpress 4xA9) and
64bit platforms(RTSMv8) running 32bit Android userspace and 64bit Linux +
ashmem unit tests.

Best Regards,
Serban Constantinescu
PDSW Engineer ARM Ltd.


Serban Constantinescu (2):
  staging: android: ashmem: fix ashmem pin/unpin interface
  staging: android: ashmem: Add support for 32bit ashmem calls in a
    64bit kernel

 drivers/staging/android/ashmem.c |   65 ++++++++++++++++++++++++++++++--------
 drivers/staging/android/ashmem.h |   19 +++++++++--
 2 files changed, 68 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface

[Serban Constantinescu]

The values exchanged between kernel and userspace through struct
ashmem_pin should be of type size_t. This change won't affect the
existing interface but will stand as the basis of 64bit compat layer.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/ashmem.c |    2 +-
 drivers/staging/android/ashmem.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 72064fc..aa52646 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -596,7 +596,7 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
 	if (unlikely((pin.offset | pin.len) & ~PAGE_MASK))
 		return -EINVAL;
 
-	if (unlikely(((__u32) -1) - pin.offset < pin.len))
+	if (unlikely(((size_t) -1) - pin.offset < pin.len))
 		return -EINVAL;
 
 	if (unlikely(PAGE_ALIGN(asma->size) < pin.offset + pin.len))
diff --git a/drivers/staging/android/ashmem.h b/drivers/staging/android/ashmem.h
index 1976b10..c9b2eba 100644
--- a/drivers/staging/android/ashmem.h
+++ b/drivers/staging/android/ashmem.h
@@ -28,8 +28,8 @@
 #define ASHMEM_IS_PINNED	1
 
 struct ashmem_pin {
-	__u32 offset;	/* offset into region, in bytes, page-aligned */
-	__u32 len;	/* length forward from offset, in bytes, page-aligned */
+	size_t offset;	/* offset into region, in bytes, page-aligned */
+	size_t len;	/* length forward from offset, in bytes, page-aligned */
 };
 
 #define __ASHMEMIOC		0x77
-- 
1.7.9.5

Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface

[Greg KH]

On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
> The values exchanged between kernel and userspace through struct
> ashmem_pin should be of type size_t. This change won't affect the
> existing interface but will stand as the basis of 64bit compat layer.

How do you define size_t with a 64bit kernel and a 32bit userspace
properly?  Doesn't this change open up a bunch of problems?

thanks,

greg k-h

Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface

[Serban Constantinescu]

Hi Greg,

On 01/02/13 16:18, Greg KH wrote:
> On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
>> The values exchanged between kernel and userspace through struct
>> ashmem_pin should be of type size_t. This change won't affect the
>> existing interface but will stand as the basis of 64bit compat layer.
>
> How do you define size_t with a 64bit kernel and a 32bit userspace
> properly?  Doesn't this change open up a bunch of problems?

The current ashmem pin/unpin kernel interface uses __u32 to specify the 
memory region and length in bytes. However these values  should be of 
type size_t so that they are able to represent the whole range of 
possible values when compiled for a 64bit platform.

Android API uses ashmem driver through libcutils, from where I attach 
the following snippet:

<aosp>/system/core/libcutils/ashmem-dev.c

   75 int ashmem_pin_region(int fd, size_t offset, size_t len)
   76 {
   77         struct ashmem_pin pin = { offset, len };
   78         return ioctl(fd, ASHMEM_PIN, &pin);
   79 }

The kernel changes inline with the userspace usage and do not affect 
existing 32bit Android (we have exported the new kernel header, rebuilt 
and tested the interface with success).

However this change will affect any 64bit userspace using the current 
faulty interface, but there is none that we know of.

Thanks,
Serban Constantinescu
`

Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface

[Greg KH]

On Fri, Feb 01, 2013 at 04:55:01PM +0000, Serban Constantinescu wrote:
> Hi Greg,
> 
> On 01/02/13 16:18, Greg KH wrote:
> >On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
> >>The values exchanged between kernel and userspace through struct
> >>ashmem_pin should be of type size_t. This change won't affect the
> >>existing interface but will stand as the basis of 64bit compat layer.
> >
> >How do you define size_t with a 64bit kernel and a 32bit userspace
> >properly?  Doesn't this change open up a bunch of problems?
> 
> The current ashmem pin/unpin kernel interface uses __u32 to specify
> the memory region and length in bytes. However these values  should
> be of type size_t so that they are able to represent the whole range
> of possible values when compiled for a 64bit platform.

Yes, the issue is, what size is size_t on the system if you have a 32bit
userspace and a 64bit kernel?  :)

That's why we have specific types for when we cross the user/kernel
boundry.  Why not use them instead here so that you know it will work
properly in the future?

> Android API uses ashmem driver through libcutils, from where I
> attach the following snippet:
> 
> <aosp>/system/core/libcutils/ashmem-dev.c
> 
>   75 int ashmem_pin_region(int fd, size_t offset, size_t len)
>   76 {
>   77         struct ashmem_pin pin = { offset, len };
>   78         return ioctl(fd, ASHMEM_PIN, &pin);
>   79 }

Again, the 32/64 bit issue is to blame.

> The kernel changes inline with the userspace usage and do not affect
> existing 32bit Android (we have exported the new kernel header,
> rebuilt and tested the interface with success).
> 
> However this change will affect any 64bit userspace using the
> current faulty interface, but there is none that we know of.

I'd like the Android developers to give some feedback on this, before
I'll do anything.  I still think you need to change this to use the
proper kernel types.

thanks,

greg k-h

Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface

[Serban Constantinescu]

On 04/02/13 01:41, Greg KH wrote:
> On Fri, Feb 01, 2013 at 04:55:01PM +0000, Serban Constantinescu wrote:
>> Hi Greg,
>>
>> On 01/02/13 16:18, Greg KH wrote:
>>> On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
>>>> The values exchanged between kernel and userspace through struct
>>>> ashmem_pin should be of type size_t. This change won't affect the
>>>> existing interface but will stand as the basis of 64bit compat layer.
>>>
>>> How do you define size_t with a 64bit kernel and a 32bit userspace
>>> properly?  Doesn't this change open up a bunch of problems?
>>
>> The current ashmem pin/unpin kernel interface uses __u32 to specify
>> the memory region and length in bytes. However these values  should
>> be of type size_t so that they are able to represent the whole range
>> of possible values when compiled for a 64bit platform.
>
> Yes, the issue is, what size is size_t on the system if you have a 32bit
> userspace and a 64bit kernel?  :)
>
> That's why we have specific types for when we cross the user/kernel
> boundry.  Why not use them instead here so that you know it will work
> properly in the future?

The patch set was developed using size_t to avoid the use of a #ifdef
statement that would split the use for 64bit platforms using u64 and
32bit using u32. On a 64/32 system you will have a 32bit size_t in the
user space and a 64bit size_t in the kernel. However in the kernel entry 
- compat_ashmem_ioctl we explicitly cast from compat_size_t (32bit - 
same as the userspace) to size_t (64bit used by the kernel).

 > 695                 pin.offset = (size_t)c_pin.offset;
 > 696                 pin.len = (size_t)c_pin.len;

Same logic was applied for existing size_t ioctls:

 > 46 #define ASHMEM_SET_SIZE         _IOW(__ASHMEMIOC, 3, size_t)

where on a 64/32 system you are using:

 > 57 #define COMPAT_ASHMEM_SET_SIZE          _IOW(__ASHMEMIOC, 3, 
compat_size_t)

This kernel header is not intended to be exported for current Android
userspace (even though we have tested this with success on one of the
latest Android revisions).

>> Android API uses ashmem driver through libcutils, from where I
>> attach the following snippet:
>>
>> <aosp>/system/core/libcutils/ashmem-dev.c
>>
>>    75 int ashmem_pin_region(int fd, size_t offset, size_t len)
>>    76 {
>>    77         struct ashmem_pin pin = { offset, len };
>>    78         return ioctl(fd, ASHMEM_PIN, &pin);
>>    79 }
>
> Again, the 32/64 bit issue is to blame.
>
>> The kernel changes inline with the userspace usage and do not affect
>> existing 32bit Android (we have exported the new kernel header,
>> rebuilt and tested the interface with success).
>>
>> However this change will affect any 64bit userspace using the
>> current faulty interface, but there is none that we know of.
>
> I'd like the Android developers to give some feedback on this, before
> I'll do anything.  I still think you need to change this to use the
> proper kernel types.

If you or the Android developers consider the use of #ifdef u64/u32 is 
better I will rework the patch accordingly.


Thanks,
Serban Constantinescu

[PATCH 2/2] staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel

[Serban Constantinescu]

Android's shared memory subsystem, Ashmem, does not support calls from a
32bit userspace in a 64 bit kernel. This patch adds support for syscalls
coming from a 32bit userspace in a 64bit kernel.

The patch has been successfully tested on ARMv8 AEM(64bit
platform model) and Versatile Express A9(32bit platform).

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/ashmem.c |   65 ++++++++++++++++++++++++++++++--------
 drivers/staging/android/ashmem.h |   15 +++++++++
 2 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index aa52646..75b08a9 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -577,33 +577,29 @@ static int ashmem_get_pin_status(struct ashmem_area *asma, size_t pgstart,
 }
 
 static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
-			    void __user *p)
+			    struct ashmem_pin *pin)
 {
-	struct ashmem_pin pin;
 	size_t pgstart, pgend;
 	int ret = -EINVAL;
 
 	if (unlikely(!asma->file))
 		return -EINVAL;
 
-	if (unlikely(copy_from_user(&pin, p, sizeof(pin))))
-		return -EFAULT;
-
 	/* per custom, you can pass zero for len to mean "everything onward" */
-	if (!pin.len)
-		pin.len = PAGE_ALIGN(asma->size) - pin.offset;
+	if (!pin->len)
+		pin->len = PAGE_ALIGN(asma->size) - pin->offset;
 
-	if (unlikely((pin.offset | pin.len) & ~PAGE_MASK))
+	if (unlikely((pin->offset | pin->len) & ~PAGE_MASK))
 		return -EINVAL;
 
-	if (unlikely(((size_t) -1) - pin.offset < pin.len))
+	if (unlikely(((size_t) -1) - pin->offset < pin->len))
 		return -EINVAL;
 
-	if (unlikely(PAGE_ALIGN(asma->size) < pin.offset + pin.len))
+	if (unlikely(PAGE_ALIGN(asma->size) < pin->offset + pin->len))
 		return -EINVAL;
 
-	pgstart = pin.offset / PAGE_SIZE;
-	pgend = pgstart + (pin.len / PAGE_SIZE) - 1;
+	pgstart = pin->offset / PAGE_SIZE;
+	pgend = pgstart + (pin->len / PAGE_SIZE) - 1;
 
 	mutex_lock(&ashmem_mutex);
 
@@ -627,6 +623,7 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
 static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct ashmem_area *asma = file->private_data;
+	struct ashmem_pin pin;
 	long ret = -ENOTTY;
 
 	switch (cmd) {
@@ -655,7 +652,9 @@ static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case ASHMEM_PIN:
 	case ASHMEM_UNPIN:
 	case ASHMEM_GET_PIN_STATUS:
-		ret = ashmem_pin_unpin(asma, cmd, (void __user *) arg);
+		if (unlikely(copy_from_user(&pin, (void __user *)arg, sizeof(pin))))
+			return -EFAULT;
+		ret = ashmem_pin_unpin(asma, cmd, &pin);
 		break;
 	case ASHMEM_PURGE_ALL_CACHES:
 		ret = -EPERM;
@@ -674,6 +673,42 @@ static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return ret;
 }
 
+/* support of 32bit userspace on 64bit platforms */
+#ifdef CONFIG_COMPAT
+static long compat_ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct compat_ashmem_pin c_pin;
+	struct ashmem_pin pin;
+
+	switch (cmd) {
+	case COMPAT_ASHMEM_SET_SIZE:
+		cmd = ASHMEM_SET_SIZE;
+		break;
+	case COMPAT_ASHMEM_SET_PROT_MASK:
+		cmd = ASHMEM_SET_PROT_MASK;
+		break;
+	case COMPAT_ASHMEM_PIN:
+	case COMPAT_ASHMEM_UNPIN:
+	case ASHMEM_GET_PIN_STATUS:
+		if (unlikely(copy_from_user(&c_pin, (void __user *)arg, sizeof(c_pin))))
+			return -EFAULT;
+		pin.offset = (size_t)c_pin.offset;
+		pin.len = (size_t)c_pin.len;
+		switch (cmd) {
+		case COMPAT_ASHMEM_PIN:
+			cmd = ASHMEM_PIN;
+			break;
+		case COMPAT_ASHMEM_UNPIN:
+			cmd = ASHMEM_UNPIN;
+			break;
+		}
+		return ashmem_pin_unpin(file->private_data, cmd, &pin);
+	}
+
+	return ashmem_ioctl(file, cmd, arg);
+}
+#endif /* CONFIG_COMPAT */
+
 static const struct file_operations ashmem_fops = {
 	.owner = THIS_MODULE,
 	.open = ashmem_open,
@@ -682,7 +717,9 @@ static const struct file_operations ashmem_fops = {
 	.llseek = ashmem_llseek,
 	.mmap = ashmem_mmap,
 	.unlocked_ioctl = ashmem_ioctl,
-	.compat_ioctl = ashmem_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = compat_ashmem_ioctl,
+#endif
 };
 
 static struct miscdevice ashmem_misc = {
diff --git a/drivers/staging/android/ashmem.h b/drivers/staging/android/ashmem.h
index c9b2eba..ff7328f 100644
--- a/drivers/staging/android/ashmem.h
+++ b/drivers/staging/android/ashmem.h
@@ -32,6 +32,13 @@ struct ashmem_pin {
 	size_t len;	/* length forward from offset, in bytes, page-aligned */
 };
 
+#ifdef CONFIG_COMPAT
+struct compat_ashmem_pin {
+	compat_size_t offset;	/* offset into region, in bytes, page-aligned */
+	compat_size_t len;	/* length forward from offset, in bytes, page-aligned */
+};
+#endif
+
 #define __ASHMEMIOC		0x77
 
 #define ASHMEM_SET_NAME		_IOW(__ASHMEMIOC, 1, char[ASHMEM_NAME_LEN])
@@ -45,4 +52,12 @@ struct ashmem_pin {
 #define ASHMEM_GET_PIN_STATUS	_IO(__ASHMEMIOC, 9)
 #define ASHMEM_PURGE_ALL_CACHES	_IO(__ASHMEMIOC, 10)
 
+/* support of 32bit userspace on 64bit platforms */
+#ifdef CONFIG_COMPAT
+#define COMPAT_ASHMEM_SET_SIZE		_IOW(__ASHMEMIOC, 3, compat_size_t)
+#define COMPAT_ASHMEM_SET_PROT_MASK	_IOW(__ASHMEMIOC, 5, unsigned int)
+#define COMPAT_ASHMEM_PIN		_IOW(__ASHMEMIOC, 7, struct compat_ashmem_pin)
+#define COMPAT_ASHMEM_UNPIN		_IOW(__ASHMEMIOC, 8, struct compat_ashmem_pin)
+#endif
+
 #endif	/* _LINUX_ASHMEM_H */
-- 
1.7.9.5