[RESEND PATCH V4] staging: vchiq_arm: Add compatibility wrappers for ioctls

[Michael Zoran]

On Mon, 2017-03-06 at 18:22 +0300, Dan Carpenter wrote:
> > +
> > +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;
> > +
> > +	ret = vchiq_ioctl(file,
> > +			  VCHIQ_IOC_AWAIT_COMPLETION,
> > +			  (unsigned long)args);
> > +
> > +	if (ret <= 0)
> 
> Really?
> 

Believe it or not, this code actually does work.  It's the original
code that's convoluted.  

By forcing 1 for args->count and args->msgbufcount, it avoids all the
broken code paths in the original code.  

Here is a brief summary of what the orignal code returns:

0: No messages where available.
>0: Number of messages that were availably but never more then args-
>count.
<0: An error occured, but only if only 1 message was available. 
Otherwise return the count of available messages.

> > +		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;
> > +
> > +	if (copy_to_user(msgbufcount32,
> > +			 &args32.msgbufcount,
> > +			 sizeof(args32.msgbufcount)))
> > +		return -EFAULT;
> > +
> > +	return ret;
> 
> 	return 0;
> 


The other fixes are very easy to make and I have no problem changes
those.