Michael Zoran <mzoran@xxxxxxxxxxxx> writes:
> This patch adds compatibility wrappers for the ioctls
> exposed by vchiq/vc04_services. The compat ioctls are
> completely implemented on top of the native ioctls. No
> existing lines are modified.
>
> While the ideal approach would be to cleanup the existing
> code, this path is simplier and easier to review. While
> it does have a small runtime performance penality vs
> seperating the existing code into wrapper+worker functions,
> the penality is small since only the metadata is copied
> back onto the 32 bit user mode stack.
>
> The on top of approach is the approach used by several
> existing performance critical subsystems of Linux such
> as the DRM 3D graphics subsystem.
>
> Testing:
>
> 1. A 32 bit chroot was created on a RPI 3 and vchiq_test
> was built for armhf. The usual tests were run such as
> vchiq_test -f 10 and vchiq_test -p.
>
> 2. This patch was applied to the shipping version of
> the Linux kernel used for the RPI and that kernel was
> built for arm64. That kernel was used to boot Raspbian.
> Many of the builtin features are now functional such
> as the "hello_pi" examples, and minecraft_pi.
> Signed-off-by: Michael Zoran <mzoran@xxxxxxxxxxxx>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 446 +++++++++++++++++++++
> 1 file changed, 446 insertions(+)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 19bd4ac6e855..90dfa79089d3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -47,6 +47,7 @@
> #include <linux/list.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/compat.h>
> #include <soc/bcm2835/raspberrypi-firmware.h>
>
> #include "vchiq_core.h"
> @@ -1227,6 +1228,448 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> return ret;
> }
>
> +#if defined(CONFIG_COMPAT)
> +static long
> +vchiq_compat_ioctl_queue_message(struct file *file,
> + unsigned int cmd,
> + unsigned long arg)
> +{
> + VCHIQ_QUEUE_MESSAGE_T *args;
> + VCHIQ_ELEMENT_T *elements;
> + struct vchiq_queue_message32 args32;
> + unsigned int count;
> +
> + if (copy_from_user(&args32,
> + (struct vchiq_queue_message32 __user *)arg,
> + sizeof(args32)))
> + return -EFAULT;
> +
> + args = compat_alloc_user_space(sizeof(*args) +
> + (sizeof(*elements) * MAX_ELEMENTS));
> +
> + if (!args)
> + return -EFAULT;
> +
> + if (put_user(args32.handle, &args->handle) ||
> + put_user(args32.count, &args->count) ||
> + put_user(compat_ptr(args32.elements), &args->elements))
> + return -EFAULT;
> +
> + if (args32.elements && args32.count && !(args32.count > MAX_ELEMENTS)) {
> + struct vchiq_element32 tempelement32[MAX_ELEMENTS];
> +
> + elements = (VCHIQ_ELEMENT_T __user *)(args + 1);
> +
> + if (copy_from_user(&tempelement32,
> + compat_ptr(args32.elements),
> + sizeof(tempelement32)))
> + return -EFAULT;
> +
> + for (count = 0; count < args32.count; count++) {
> + if (put_user(compat_ptr(tempelement32[count].data),
> + &elements[count].data) ||
> + put_user(tempelement32[count].size,
> + &elements[count].size))
> + return -EFAULT;
> + }
> +
> + if (put_user(elements, &args->elements))
> + return -EFAULT;
> + }
I think inside of this block you should just check args32.count >
MAX_ELEMENTS and return -EINVAL in that case, rather than silently not
copying the elements.
> +
> + return vchiq_ioctl(file, VCHIQ_IOC_QUEUE_MESSAGE, (unsigned long)args);
> +}
> +
> +struct vchiq_completion_data32 {
> + VCHIQ_REASON_T reason;
> + compat_uptr_t header;
> + compat_uptr_t service_userdata;
> + compat_uptr_t bulk_userdata;
> +};
> +
> +struct vchiq_await_completion32 {
> + unsigned int count;
> + compat_uptr_t buf;
> + unsigned int msgbufsize;
> + unsigned int msgbufcount; /* IN/OUT */
> + compat_uptr_t msgbufs;
> +};
> +
> +#define VCHIQ_IOC_AWAIT_COMPLETION32 \
> + _IOWR(VCHIQ_IOC_MAGIC, 7, struct vchiq_await_completion32)
> +
> +static long
> +vchiq_compat_ioctl_await_completion(struct file *file,
> + unsigned int cmd,
> + unsigned long arg)
> +{
> + VCHIQ_AWAIT_COMPLETION_T *args;
> + VCHIQ_COMPLETION_DATA_T *completion;
> + VCHIQ_COMPLETION_DATA_T completiontemp;
> + struct vchiq_await_completion32 args32;
> + struct vchiq_completion_data32 completion32;
> + unsigned int *msgbufcount32;
> + compat_uptr_t msgbuf32;
> + void *msgbuf;
> + void **msgbufptr;
> + long ret;
> +
> + args = compat_alloc_user_space(sizeof(*args) +
> + sizeof(*completion) +
> + sizeof(*msgbufptr));
> + if (!args)
> + return -EFAULT;
> +
> + completion = (VCHIQ_COMPLETION_DATA_T *)(args + 1);
> + msgbufptr = (void __user **)(completion + 1);
> +
> + if (copy_from_user(&args32,
> + (struct vchiq_completion_data32 *)arg,
> + sizeof(args32)))
> + return -EFAULT;
> +
> + if (put_user(args32.count, &args->count) ||
> + put_user(compat_ptr(args32.buf), &args->buf) ||
> + put_user(args32.msgbufsize, &args->msgbufsize) ||
> + put_user(args32.msgbufcount, &args->msgbufcount) ||
> + put_user(compat_ptr(args32.msgbufs), &args->msgbufs))
> + return -EFAULT;
> +
> + if (!args32.count || !args32.buf || !args32.msgbufcount)
> + return vchiq_ioctl(file,
> + VCHIQ_IOC_AWAIT_COMPLETION,
> + (unsigned long)args);
> +
> + if (copy_from_user(&msgbuf32,
> + compat_ptr(args32.msgbufs) +
> + (sizeof(compat_uptr_t) *
> + (args32.msgbufcount - 1)),
> + sizeof(msgbuf32)))
> + return -EFAULT;
> +
> + msgbuf = compat_ptr(msgbuf32);
> +
> + if (copy_to_user(msgbufptr,
> + &msgbuf,
> + sizeof(msgbuf)))
> + return -EFAULT;
> +
> + if (copy_to_user(&args->msgbufs,
> + &msgbufptr,
> + sizeof(msgbufptr)))
> + return -EFAULT;
> +
> + if (put_user(1U, &args->count) ||
> + put_user(completion, &args->buf) ||
> + put_user(1U, &args->msgbufcount))
> + return -EFAULT;
Seems like instead of treating the user ioctl as having specified a
count of 1 / msgbufcount of 1, we should throw -EINVAL if they specified
something other than what we support for the compat path.
(this also means we don't need the weird bumping of msgbuf by
msgbufcount - 1 in the copy_from_user above, right?)
> +
> + ret = vchiq_ioctl(file,
> + VCHIQ_IOC_AWAIT_COMPLETION,
> + (unsigned long)args);
> +
> + if (ret <= 0)
> + return ret;
> +
> + if (copy_from_user(&completiontemp, completion, sizeof(*completion)))
> + return -EFAULT;
> +
> + completion32.reason = completiontemp.reason;
> + completion32.header = ptr_to_compat(completiontemp.header);
> + completion32.service_userdata =
> + ptr_to_compat(completiontemp.service_userdata);
> + completion32.bulk_userdata =
> + ptr_to_compat(completiontemp.bulk_userdata);
> +
> + if (copy_to_user(compat_ptr(args32.buf),
> + &completion32,
> + sizeof(completion32)))
> + return -EFAULT;
> +
> + args32.msgbufcount--;
> +
> + msgbufcount32 =
> + &((struct vchiq_await_completion32 __user *)arg)->msgbufcount;
There seem to be conditions in the real ioctl where msgbufcount doesn't
get decremented. Could we just get_user() the args->msgbufcount and
copy that back out?
With these 3 fixes,
Reviewed-by: Eric Anholt <eric@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
