[PATCH 4/9] VC04_SERVICES: Add compat ioctl handler for "queue message"

[Dan Carpenter]

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 at crowfest.net>
> ---
>  .../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