On Wed, Jan 18, 2017 at 07:04:48AM -0800, Michael Zoran wrote:
> Add compat handler for "queue message" ioctl.
>
> Signed-off-by: Michael Zoran <mzoran@xxxxxxxxxxxx>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 36 ++++++++++++++++++++++
> 1 file changed, 36 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 e26949247f91..1c0afa318036 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1271,6 +1271,41 @@ vchiq_ioctl_compat_internal(
> }
> } break;
>
> + case VCHIQ_IOC_QUEUE_MESSAGE32: {
> + struct vchiq_queue_message32 args32;
To be honest, it would probably be easier if we introduced the
vchiq_queue_message32 in this patch instead of patch 1 so I can review
it without flipping back and forth in my emails.
> + VCHIQ_ELEMENT_T elements[MAX_ELEMENTS];
> + struct vchiq_element32 elements32[MAX_ELEMENTS];
> + unsigned int i;
Just "int i". No need to get fancy.
> +
> + if (copy_from_user
> + (&args32, (const void __user *)arg,
Cast at the beginning. No need for these consts.
> + sizeof(args32))) {
> + ret = -EFAULT;
> + break;
return -EFAULT;
> + }
> +
> + service = find_service_for_instance(instance, args32.handle);
> +
> + if (!service || args32.count > MAX_ELEMENTS) {
Move the "args32.count > MAX_ELEMENTS" directly after the copy.
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (copy_from_user(elements32, compat_ptr(args32.elements),
> + args32.count * sizeof(struct vchiq_element32))) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + for (i = 0; i < args32.count; i++) {
> + elements[i].data = compat_ptr(elements32[i].data);
> + elements[i].size = elements32[i].size;
> + }
> +
> + status = vchiq_ioc_queue_message(args32.handle,
> + elements, args32.count);
Btw, I notice that in vchiq_ioc_queue_message() "total_size +=
elements[i].size;" can have an integer overflow. What are the security
implications of that?
We're not checking "status" here and that's probably a bug.
> + } break;
Move the break.
This patch is basically OK.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
