Re: [PATCH 12/12] v4l2-compat-ioctl32.c: refactor, fix security bug in compat ioctl32

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Sakari,

On 01/29/2018 06:06 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for your efforts on this patch and the patchset. Please see my comments below.
> 
> On Fri, Jan 26, 2018 at 01:43:27PM +0100, Hans Verkuil wrote:
>> From: Daniel Mentz <danielmentz@google.com>
>>
>> The 32-bit compat v4l2 ioctl is implemented based on its 64-bit
> 
> s/v4l2 ioctl/V4L2 IOCTL handling/
> 
> ?
> 
>> equivalent. It converts 32-bit data structures into its 64-bit
>> equivalents and needs to provide the data to the 64-bit ioctl in user
>> space memory which is commonly allocated using
>> compat_alloc_user_space(). However, due to how that function is
>> implemented, it can only be called a single time for every syscall
>> invocation.  Supposedly to avoid this limitation, the existing code uses
>> a mix of memory from the kernel stack and memory allocated through
>> compat_alloc_user_space(). Under normal circumstances, this would not
>> work, because the 64-bit ioctl expects all pointers to point to user
>> space memory. As a workaround, set_fs(KERNEL_DS) is called to
>> temporarily disable this extra safety check and allow kernel pointers.
>> However, this might introduce a security vulnerability: The
>> result of the 32-bit to 64-bit conversion is writeable by user space
>> because the output buffer has been allocated via
>> compat_alloc_user_space(). A malicious user space process could then
>> manipulate pointers inside this output buffer, and due to the previous
>> set_fs(KERNEL_DS) call, functions like get_user() or put_user() no longer
>> prevent kernel memory access.
>>
>> The new approach is to pre-calculate the total amount of user space
>> memory that is needed, allocate it using compat_alloc_user_space() and
>> then divide up the allocated memory to accommodate all data structures
>> that need to be converted.
>>
>> An alternative approach would have been to retain the union type karg
>> that they allocated on the kernel stack in do_video_ioctl(), copy all
>> data from user space into karg and then back to user space. However,
>> we decided against this approach because it does not align with other
>> compat syscall implementations. Instead, we tried to replicate the
>> get_user/put_user pairs as found in other places in the kernel:
>>
>> if (get_user(clipcount, &up->clipcount) ||
>>     put_user(clipcount, &kp->clipcount)) return -EFAULT;
>>
>> Notes from hans.verkuil@cisco.com:
>>
>> This patch was taken from
>> https://github.com/LineageOS/android_kernel_samsung_apq8084/commit/97b733953c06e4f0398ade18850f0817778255f7
>>
>> Clearly nobody could be bothered to upstream this patch or at minimum
>> tell us :-( We only heard about this a week ago.
> 
> This is very sad indeed. I would really hope that we'll never run into
> something this again!
> 
>>
>> This patch was rebased and cleaned up. Compared to the original I
>> also swapped the order of the convert_in_user arguments so that they
>> matched copy_in_user. It was hard to review otherwise. I also replaced
>> the ALLOC_USER_SPACE/ALLOC_AND_GET by a normal function.
> 
> The result looks more complicated than it should be.
> 
> I wonder if the result could be made cleaner by separating the argument
> checking and copying from / to user space in video_usercopy. That
> separation almost completely exists already. Rather than mangling the
> compat argument struct to look like a user-provided 64-bit argument struct,
> copying the arguments from and to the user space would be separately done
> to 32-bit compat IOCTL arguments.
> 
> I suppose that wouldn't be a trivial change either, so for now I'd maintain
> the approach and later consider how to make this more maintainable in the
> future. Still, that approach is quite different and it'd be easier to
> switch from status quo rather than this last patch, suggesting to start
> with a revert, should we embark into this direction.
> 
> What do you think?
> 
>>
>> Signed-off-by: Daniel Mentz <danielmentz@google.com>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 700 ++++++++++++++++----------
>>  1 file changed, 448 insertions(+), 252 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 2aa9b43daf60..27a5a0961cbd 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -22,6 +22,14 @@
>>  #include <media/v4l2-ctrls.h>
>>  #include <media/v4l2-ioctl.h>
>>  
>> +/* Use the same argument order as copy_in_user */
>> +#define convert_in_user(to, from)			\
> 
> This doesn't really convert anything. How about calling it assign_in_user,
> for example?

Done.

> 
>> +({							\
>> +	typeof(*from) val;				\
> 
> "val" is very short, easily leading to hard-to-find namespace collisions.
> At least the versions of GCC I've used happily get this wrong without a
> warning.
> 
> How about __convert_in_user_tmp instead?

I stuck to __assign_tmp, I think __convert_in_user_tmp is overly paranoid :-)

> 
>> +							\
>> +	get_user(val, from) || put_user(val, to);	\

I'm keeping this. I thought about changing || to '? :' but I chickened out making
that change. I'll fix the G/S_INPUT/OUTPUT handling instead.

>> +})
>> +
>>  static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>  {
>>  	long ret = -ENOIOCTLCMD;
>> @@ -48,37 +56,41 @@ struct v4l2_window32 {
>>  	__u8                    global_alpha;
>>  };
>>  
>> -static int get_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32 __user *up)
>> +static int get_v4l2_window32(struct v4l2_window __user *kp,
>> +			     struct v4l2_window32 __user *up,
>> +			     void __user *aux_buf, int aux_space)
>>  {
>>  	struct v4l2_clip32 __user *uclips;
>>  	struct v4l2_clip __user *kclips;
>>  	compat_caddr_t p;
>> -	u32 n;
>> +	u32 clipcount;
>>  
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> -	    copy_from_user(&kp->w, &up->w, sizeof(up->w)) ||
>> -	    get_user(kp->field, &up->field) ||
>> -	    get_user(kp->chromakey, &up->chromakey) ||
>> -	    get_user(kp->clipcount, &up->clipcount) ||
>> -	    get_user(kp->global_alpha, &up->global_alpha))
>> +	    copy_in_user(&kp->w, &up->w, sizeof(up->w)) ||
>> +	    convert_in_user(&kp->field, &up->field) ||
>> +	    convert_in_user(&kp->chromakey, &up->chromakey) ||
>> +	    convert_in_user(&kp->global_alpha, &up->global_alpha) ||
>> +	    get_user(clipcount, &up->clipcount) ||
>> +	    put_user(clipcount, &kp->clipcount))
>>  		return -EFAULT;
>> -	if (kp->clipcount > 2048)
>> +	if (clipcount > 2048)
>>  		return -EINVAL;
>> -	if (!kp->clipcount) {
>> -		kp->clips = NULL;
>> -		return 0;
>> -	}
>> +	if (!clipcount)
>> +		return put_user(NULL, &kp->clips) ? -EFAULT : 0;
> 
> Isn't this what put_user would return anyway?

Changed.

> 
>>  
>> -	n = kp->clipcount;
>>  	if (get_user(p, &up->clips))
>>  		return -EFAULT;
>>  	uclips = compat_ptr(p);
>> -	kclips = compat_alloc_user_space(n * sizeof(*kclips));
>> -	kp->clips = kclips;
>> -	while (n--) {
>> +	if (aux_space < clipcount * sizeof(*kclips))
>> +		return -EFAULT;
>> +	kclips = aux_buf;
>> +	if (put_user(kclips, &kp->clips))
>> +		return -EFAULT;
>> +
>> +	while (clipcount--) {
>>  		if (copy_in_user(&kclips->c, &uclips->c, sizeof(uclips->c)))
>>  			return -EFAULT;
>> -		if (put_user(n ? kclips + 1 : NULL, &kclips->next))
>> +		if (put_user(clipcount ? kclips + 1 : NULL, &kclips->next))
>>  			return -EFAULT;
>>  		uclips++;
>>  		kclips++;
>> @@ -86,26 +98,28 @@ static int get_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32 __user
>>  	return 0;
>>  }
>>  
>> -static int put_v4l2_window32(struct v4l2_window *kp, struct v4l2_window32 __user *up)
>> +static int put_v4l2_window32(struct v4l2_window __user *kp,
>> +			     struct v4l2_window32 __user *up)
>>  {
>>  	struct v4l2_clip __user *kclips = kp->clips;
>>  	struct v4l2_clip32 __user *uclips;
>> -	int n = kp->clipcount;
>>  	compat_caddr_t p;
>> -
>> -	if (copy_to_user(&up->w, &kp->w, sizeof(kp->w)) ||
>> -	    put_user(kp->field, &up->field) ||
>> -	    put_user(kp->chromakey, &up->chromakey) ||
>> -	    put_user(kp->clipcount, &up->clipcount) ||
>> -	    put_user(kp->global_alpha, &up->global_alpha))
>> +	u32 clipcount;
>> +
>> +	if (copy_in_user(&up->w, &kp->w, sizeof(kp->w)) ||
>> +	    convert_in_user(&up->field, &kp->field) ||
>> +	    convert_in_user(&up->chromakey, &kp->chromakey) ||
>> +	    convert_in_user(&up->global_alpha, &kp->global_alpha) ||
>> +	    get_user(clipcount, &kp->clipcount) ||
>> +	    put_user(clipcount, &up->clipcount))
>>  		return -EFAULT;
>> -	if (!kp->clipcount)
>> +	if (!clipcount)
>>  		return 0;
>>  
>>  	if (get_user(p, &up->clips))
>>  		return -EFAULT;
>>  	uclips = compat_ptr(p);
>> -	while (n--) {
>> +	while (clipcount--) {
>>  		if (copy_in_user(&uclips->c, &kclips->c, sizeof(uclips->c)))
>>  			return -EFAULT;
>>  		uclips++;
>> @@ -145,107 +159,161 @@ struct v4l2_create_buffers32 {
>>  	__u32			reserved[8];
>>  };
>>  
>> -static int __get_v4l2_format32(struct v4l2_format *kp, struct v4l2_format32 __user *up)
>> +static int __bufsize_v4l2_format32(struct v4l2_format32 __user *up)
> 
> This is effectively returning the size of the non-compat struct, or just
> the part of which is variable. How about calling it __bufsize_v4l2_format
> instead? The same for other such functions.

Done.

> 
> I'd also separate the size calculated by the function and the return value.

Done. Much nicer indeed.

> 
>>  {
>> -	if (get_user(kp->type, &up->type))
>> +	u32 type;
>> +
>> +	if (get_user(type, &up->type))
>>  		return -EFAULT;
>>  
>> -	switch (kp->type) {
>> +	switch (type) {
>> +	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: {
>> +		u32 clipcount;
>> +
>> +		if (get_user(clipcount, &up->fmt.win.clipcount))
>> +			return -EFAULT;
>> +		if (clipcount > 2048)
>> +			return -EINVAL;
>> +		return clipcount * sizeof(struct v4l2_clip);
>> +	}
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int bufsize_v4l2_format32(struct v4l2_format32 __user *up)
>> +{
>> +	if (!access_ok(VERIFY_READ, up, sizeof(*up)))
>> +		return -EFAULT;
> 
> Newline?

Didn't do this. I didn't think it improved the code.

> 
>> +	return __bufsize_v4l2_format32(up);
>> +}
>> +
>> +static int __get_v4l2_format32(struct v4l2_format __user *kp,
>> +			       struct v4l2_format32 __user *up,
>> +			       void __user *aux_buf, int aux_space)
>> +{
>> +	u32 type;
>> +
>> +	if (get_user(type, &up->type) || put_user(type, &kp->type))
>> +		return -EFAULT;
>> +
>> +	switch (type) {
>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>> -		return copy_from_user(&kp->fmt.pix, &up->fmt.pix,
>> -				      sizeof(kp->fmt.pix)) ? -EFAULT : 0;
>> +		return copy_in_user(&kp->fmt.pix, &up->fmt.pix,
>> +				    sizeof(kp->fmt.pix)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> -		return copy_from_user(&kp->fmt.pix_mp, &up->fmt.pix_mp,
>> -				      sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
>> +		return copy_in_user(&kp->fmt.pix_mp, &up->fmt.pix_mp,
>> +				    sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
>> -		return get_v4l2_window32(&kp->fmt.win, &up->fmt.win);
>> +		return get_v4l2_window32(&kp->fmt.win, &up->fmt.win,
>> +					 aux_buf, aux_space);
>>  	case V4L2_BUF_TYPE_VBI_CAPTURE:
>>  	case V4L2_BUF_TYPE_VBI_OUTPUT:
>> -		return copy_from_user(&kp->fmt.vbi, &up->fmt.vbi,
>> -				      sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
>> +		return copy_in_user(&kp->fmt.vbi, &up->fmt.vbi,
>> +				    sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
>>  	case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
>> -		return copy_from_user(&kp->fmt.sliced, &up->fmt.sliced,
>> -				      sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
>> +		return copy_in_user(&kp->fmt.sliced, &up->fmt.sliced,
>> +				    sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
>>  	case V4L2_BUF_TYPE_SDR_OUTPUT:
>> -		return copy_from_user(&kp->fmt.sdr, &up->fmt.sdr,
>> -				      sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
>> +		return copy_in_user(&kp->fmt.sdr, &up->fmt.sdr,
>> +				    sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_META_CAPTURE:
>> -		return copy_from_user(&kp->fmt.meta, &up->fmt.meta,
>> -				      sizeof(kp->fmt.meta)) ? -EFAULT : 0;
>> +		return copy_in_user(&kp->fmt.meta, &up->fmt.meta,
>> +				    sizeof(kp->fmt.meta)) ? -EFAULT : 0;
>>  	default:
>>  		return -EINVAL;
>>  	}
>>  }
>>  
>> -static int get_v4l2_format32(struct v4l2_format *kp, struct v4l2_format32 __user *up)
>> +static int get_v4l2_format32(struct v4l2_format __user *kp,
>> +			     struct v4l2_format32 __user *up,
>> +			     void __user *aux_buf, int aux_space)
>> +{
>> +	if (!access_ok(VERIFY_READ, up, sizeof(*up)))
>> +		return -EFAULT;
>> +	return __get_v4l2_format32(kp, up, aux_buf, aux_space);
>> +}
>> +
>> +static int bufsize_v4l2_create32(struct v4l2_create_buffers32 __user *up)
>>  {
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)))
>>  		return -EFAULT;
>> -	return __get_v4l2_format32(kp, up);
>> +	return __bufsize_v4l2_format32(&up->format);
>>  }
>>  
>> -static int get_v4l2_create32(struct v4l2_create_buffers *kp, struct v4l2_create_buffers32 __user *up)
>> +static int get_v4l2_create32(struct v4l2_create_buffers __user *kp,
>> +			     struct v4l2_create_buffers32 __user *up,
>> +			     void __user *aux_buf, int aux_space)
>>  {
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> -	    copy_from_user(kp, up, offsetof(struct v4l2_create_buffers32, format)))
>> +	    copy_in_user(kp, up,
>> +			 offsetof(struct v4l2_create_buffers32, format)))
>>  		return -EFAULT;
>> -	return __get_v4l2_format32(&kp->format, &up->format);
>> +	return __get_v4l2_format32(&kp->format, &up->format,
>> +				   aux_buf, aux_space);
>>  }
>>  
>> -static int __put_v4l2_format32(struct v4l2_format *kp, struct v4l2_format32 __user *up)
>> +static int __put_v4l2_format32(struct v4l2_format __user *kp,
>> +			       struct v4l2_format32 __user *up)
>>  {
>> -	if (put_user(kp->type, &up->type))
>> +	u32 type;
>> +
>> +	if (get_user(type, &kp->type))
>>  		return -EFAULT;
>>  
>> -	switch (kp->type) {
>> +	switch (type) {
>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>> -		return copy_to_user(&up->fmt.pix, &kp->fmt.pix,
>> -				    sizeof(kp->fmt.pix)) ?  -EFAULT : 0;
>> +		return copy_in_user(&up->fmt.pix, &kp->fmt.pix,
>> +				    sizeof(kp->fmt.pix)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> -		return copy_to_user(&up->fmt.pix_mp, &kp->fmt.pix_mp,
>> -				    sizeof(kp->fmt.pix_mp)) ?  -EFAULT : 0;
>> +		return copy_in_user(&up->fmt.pix_mp, &kp->fmt.pix_mp,
>> +				    sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
>>  		return put_v4l2_window32(&kp->fmt.win, &up->fmt.win);
>>  	case V4L2_BUF_TYPE_VBI_CAPTURE:
>>  	case V4L2_BUF_TYPE_VBI_OUTPUT:
>> -		return copy_to_user(&up->fmt.vbi, &kp->fmt.vbi,
>> -				    sizeof(kp->fmt.vbi)) ?  -EFAULT : 0;
>> +		return copy_in_user(&up->fmt.vbi, &kp->fmt.vbi,
>> +				    sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
>>  	case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
>> -		return copy_to_user(&up->fmt.sliced, &kp->fmt.sliced,
>> -				    sizeof(kp->fmt.sliced)) ?  -EFAULT : 0;
>> +		return copy_in_user(&up->fmt.sliced, &kp->fmt.sliced,
>> +				    sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
>>  	case V4L2_BUF_TYPE_SDR_OUTPUT:
>> -		return copy_to_user(&up->fmt.sdr, &kp->fmt.sdr,
>> -				    sizeof(kp->fmt.sdr)) ?  -EFAULT : 0;
>> +		return copy_in_user(&up->fmt.sdr, &kp->fmt.sdr,
>> +				    sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
>>  	case V4L2_BUF_TYPE_META_CAPTURE:
>> -		return copy_to_user(&up->fmt.meta, &kp->fmt.meta,
>> -				    sizeof(kp->fmt.meta)) ?  -EFAULT : 0;
>> +		return copy_in_user(&up->fmt.meta, &kp->fmt.meta,
>> +				    sizeof(kp->fmt.meta)) ? -EFAULT : 0;
>>  	default:
>>  		return -EINVAL;
>>  	}
>>  }
>>  
>> -static int put_v4l2_format32(struct v4l2_format *kp, struct v4l2_format32 __user *up)
>> +static int put_v4l2_format32(struct v4l2_format __user *kp,
>> +			     struct v4l2_format32 __user *up)
>>  {
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)))
> 
> For some pointers we do perform such checks, for others we don't. While
> this patch doesn't change this, it'd be good to align this by either
> introducing it everywhere or removing it everywhere.
> 
> I guess that can be done separately.

Didn't touch this. It wouldn't hurt to review the code. I'll see if I can do
that later this week.

> 
>>  		return -EFAULT;
>>  	return __put_v4l2_format32(kp, up);
>>  }
>>  
>> -static int put_v4l2_create32(struct v4l2_create_buffers *kp, struct v4l2_create_buffers32 __user *up)
>> +static int put_v4l2_create32(struct v4l2_create_buffers __user *kp,
>> +			     struct v4l2_create_buffers32 __user *up)
>>  {
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    copy_to_user(up, kp, offsetof(struct v4l2_create_buffers32, format)) ||
>> -	    copy_to_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
>> +	    copy_in_user(up, kp,
>> +			 offsetof(struct v4l2_create_buffers32, format)) ||
>> +	    copy_in_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
>>  		return -EFAULT;
>>  	return __put_v4l2_format32(&kp->format, &up->format);
>>  }
>> @@ -259,25 +327,27 @@ struct v4l2_standard32 {
>>  	__u32		     reserved[4];
>>  };
>>  
>> -static int get_v4l2_standard32(struct v4l2_standard *kp, struct v4l2_standard32 __user *up)
>> +static int get_v4l2_standard32(struct v4l2_standard __user *kp,
>> +			       struct v4l2_standard32 __user *up)
>>  {
>>  	/* other fields are not set by the user, nor used by the driver */
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> -	    get_user(kp->index, &up->index))
>> +	    convert_in_user(&kp->index, &up->index))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>>  
>> -static int put_v4l2_standard32(struct v4l2_standard *kp, struct v4l2_standard32 __user *up)
>> +static int put_v4l2_standard32(struct v4l2_standard __user *kp,
>> +			       struct v4l2_standard32 __user *up)
>>  {
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    put_user(kp->index, &up->index) ||
>> -	    put_user(kp->id, &up->id) ||
>> -	    copy_to_user(up->name, kp->name, 24) ||
>> -	    copy_to_user(&up->frameperiod, &kp->frameperiod,
>> -			 sizeof(kp->frameperiod)) ||
>> -	    put_user(kp->framelines, &up->framelines) ||
>> -	    copy_to_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
>> +	    convert_in_user(&up->index, &kp->index) ||
>> +	    copy_in_user(&up->id, &kp->id, sizeof(up->id)) ||
> 
> Why not convert_in_user()?

Changed.

> 
>> +	    copy_in_user(up->name, kp->name, 24) ||
>> +	    copy_in_user(&up->frameperiod, &kp->frameperiod,
>> +			 sizeof(up->frameperiod)) ||
>> +	    convert_in_user(&up->framelines, &kp->framelines) ||
>> +	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>> @@ -317,10 +387,10 @@ struct v4l2_buffer32 {
>>  	__u32			reserved;
>>  };
>>  
>> -static int get_v4l2_plane32(struct v4l2_plane __user *up, struct v4l2_plane32 __user *up32,
>> +static int get_v4l2_plane32(struct v4l2_plane __user *up,
>> +			    struct v4l2_plane32 __user *up32,
>>  			    enum v4l2_memory memory)
>>  {
>> -	void __user *up_pln;
>>  	compat_long_t p;
> 
> Shouldn't this be compat_ulong_t?

Changed.

> 
>>  
>>  	if (copy_in_user(up, up32, 2 * sizeof(__u32)) ||
>> @@ -336,10 +406,8 @@ static int get_v4l2_plane32(struct v4l2_plane __user *up, struct v4l2_plane32 __
>>  			return -EFAULT;
>>  		break;
>>  	case V4L2_MEMORY_USERPTR:
>> -		if (get_user(p, &up32->m.userptr))
>> -			return -EFAULT;
>> -		up_pln = compat_ptr(p);
>> -		if (put_user((unsigned long)up_pln, &up->m.userptr))
>> +		if (get_user(p, &up32->m.userptr) ||
>> +		    put_user((unsigned long)compat_ptr(p), &up->m.userptr))
>>  			return -EFAULT;
>>  		break;
>>  	case V4L2_MEMORY_DMABUF:
>> @@ -351,7 +419,8 @@ static int get_v4l2_plane32(struct v4l2_plane __user *up, struct v4l2_plane32 __
>>  	return 0;
>>  }
>>  
>> -static int put_v4l2_plane32(struct v4l2_plane __user *up, struct v4l2_plane32 __user *up32,
>> +static int put_v4l2_plane32(struct v4l2_plane __user *up,
>> +			    struct v4l2_plane32 __user *up32,
>>  			    enum v4l2_memory memory)
>>  {
>>  	unsigned long p;
>> @@ -375,8 +444,7 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up, struct v4l2_plane32 __
>>  			return -EFAULT;
>>  		break;
>>  	case V4L2_MEMORY_DMABUF:
>> -		if (copy_in_user(&up32->m.fd, &up->m.fd,
>> -				 sizeof(up->m.fd)))
>> +		if (copy_in_user(&up32->m.fd, &up->m.fd, sizeof(up->m.fd)))
>>  			return -EFAULT;
>>  		break;
>>  	}
>> @@ -384,37 +452,68 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up, struct v4l2_plane32 __
>>  	return 0;
>>  }
>>  
>> -static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user *up)
>> +static int bufsize_v4l2_buffer32(struct v4l2_buffer32 __user *up)
>> +{
>> +	u32 type;
>> +	u32 length;
>> +
>> +	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> +	    get_user(type, &up->type) ||
>> +	    get_user(length, &up->length))
>> +		return -EFAULT;
>> +
>> +	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
>> +		if (length > VIDEO_MAX_PLANES)
>> +			return -EINVAL;
>> +
>> +		/* We don't really care if userspace decides to kill itself
> 
> /*
>  * ...

Changed here and elsewhere.

> 
>> +		 * by passing a very big length value
>> +		 */
>> +		return length * sizeof(struct v4l2_plane);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
>> +			     struct v4l2_buffer32 __user *up,
>> +			     void __user *aux_buf, int aux_space)
>>  {
>> +	u32 type;
>> +	u32 length;
>> +	enum v4l2_memory memory;
>>  	struct v4l2_plane32 __user *uplane32;
>>  	struct v4l2_plane __user *uplane;
>>  	compat_caddr_t p;
>> +	int num_planes;
> 
> u32?

Changed.

> 
>>  	int ret;
>>  
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> -	    get_user(kp->index, &up->index) ||
>> -	    get_user(kp->type, &up->type) ||
>> -	    get_user(kp->flags, &up->flags) ||
>> -	    get_user(kp->memory, &up->memory) ||
>> -	    get_user(kp->length, &up->length))
>> +	    convert_in_user(&kp->index, &up->index) ||
>> +	    get_user(type, &up->type) ||
>> +	    put_user(type, &kp->type) ||
>> +	    convert_in_user(&kp->flags, &up->flags) ||
>> +	    get_user(memory, &up->memory) ||
>> +	    put_user(memory, &kp->memory) ||
>> +	    get_user(length, &up->length) ||
>> +	    put_user(length, &kp->length))
>>  		return -EFAULT;
>>  
>> -	if (V4L2_TYPE_IS_OUTPUT(kp->type))
>> -		if (get_user(kp->bytesused, &up->bytesused) ||
>> -		    get_user(kp->field, &up->field) ||
>> -		    get_user(kp->timestamp.tv_sec, &up->timestamp.tv_sec) ||
>> -		    get_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec))
>> +	if (V4L2_TYPE_IS_OUTPUT(type))
>> +		if (convert_in_user(&kp->bytesused, &up->bytesused) ||
>> +		    convert_in_user(&kp->field, &up->field) ||
>> +		    convert_in_user(&kp->timestamp.tv_sec,
>> +				    &up->timestamp.tv_sec) ||
>> +		    convert_in_user(&kp->timestamp.tv_usec,
>> +				    &up->timestamp.tv_usec))
>>  			return -EFAULT;
>>  
>> -	if (V4L2_TYPE_IS_MULTIPLANAR(kp->type)) {
>> -		unsigned int num_planes;
>> -
>> -		if (kp->length == 0) {
>> -			kp->m.planes = NULL;
>> +	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
>> +		num_planes = length;
> 
> Do you need a local variable for just that?

I moved the u32 num_planes declaration above to here. I still kept it
though since I think it helps understand what 'length' stands for.

> 
>> +		if (num_planes == 0) {
>>  			/* num_planes == 0 is legal, e.g. when userspace doesn't
>>  			 * need planes array on DQBUF*/
>> -			return 0;
>> -		} else if (kp->length > VIDEO_MAX_PLANES) {
>> +			return put_user(NULL, &kp->m.planes);
>> +		} else if (num_planes > VIDEO_MAX_PLANES) {
>>  			return -EINVAL;
>>  		}
>>  
>> @@ -423,40 +522,47 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>>  
>>  		uplane32 = compat_ptr(p);
>>  		if (!access_ok(VERIFY_READ, uplane32,
>> -			       kp->length * sizeof(*uplane32)))
>> +				num_planes * sizeof(*uplane32)))
> 
> Alignment.

Changed.

> 
>>  			return -EFAULT;
>>  
>>  		/* We don't really care if userspace decides to kill itself
>>  		 * by passing a very big num_planes value */
>> -		uplane = compat_alloc_user_space(kp->length * sizeof(*uplane));
>> -		kp->m.planes = (__force struct v4l2_plane *)uplane;
>> +		if (aux_space < num_planes * sizeof(*uplane))
>> +			return -EFAULT;
>> +
>> +		uplane = aux_buf;
>> +		if (put_user((__force struct v4l2_plane *)uplane,
>> +					&kp->m.planes))
>> +			return -EFAULT;
>>  
>> -		for (num_planes = 0; num_planes < kp->length; num_planes++) {
>> -			ret = get_v4l2_plane32(uplane, uplane32, kp->memory);
>> +		while (--num_planes >= 0) {
> 
> while (num_planes--) {
> 
> ?

Changed.

> 
>> +			ret = get_v4l2_plane32(uplane, uplane32, memory);
>>  			if (ret)
>>  				return ret;
>> -			++uplane;
>> -			++uplane32;
>> +			uplane++;
>> +			uplane32++;
>>  		}
>>  	} else {
>> -		switch (kp->memory) {
>> +		switch (memory) {
>>  		case V4L2_MEMORY_MMAP:
>> -		case V4L2_MEMORY_OVERLAY:
>> -			if (get_user(kp->m.offset, &up->m.offset))
>> +			if (convert_in_user(&kp->m.offset, &up->m.offset))
>>  				return -EFAULT;
>>  			break;
>> -		case V4L2_MEMORY_USERPTR:
>> -			{
>> -				compat_long_t tmp;
>> -
>> -				if (get_user(tmp, &up->m.userptr))
>> -					return -EFAULT;
>> +		case V4L2_MEMORY_USERPTR: {
>> +			compat_long_t tmp;
> 
> compat_ulong_t, I'd call it "userptr". The latter is entirely up to you.

Changed.

> 
>>  
>> -				kp->m.userptr = (unsigned long)compat_ptr(tmp);
>> -			}
>> +			if (get_user(tmp, &up->m.userptr) ||
>> +			    put_user((unsigned long)compat_ptr(tmp),
>> +				     &kp->m.userptr))
>> +				return -EFAULT;
>> +			break;
>> +		}
>> +		case V4L2_MEMORY_OVERLAY:
>> +			if (convert_in_user(&kp->m.offset, &up->m.offset))
>> +				return -EFAULT;
> 
> Is there a reason to split this off from MMAP handling? The code is the
> same.

Changed.

> 
>>  			break;
>>  		case V4L2_MEMORY_DMABUF:
>> -			if (get_user(kp->m.fd, &up->m.fd))
>> +			if (convert_in_user(&kp->m.fd, &up->m.fd))
>>  				return -EFAULT;
>>  			break;
>>  		}
>> @@ -465,8 +571,12 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>>  	return 0;
>>  }
>>  
>> -static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user *up)
>> +static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>> +			     struct v4l2_buffer32 __user *up)
>>  {
>> +	u32 type;
>> +	u32 length;
>> +	enum v4l2_memory memory;
>>  	struct v4l2_plane32 __user *uplane32;
>>  	struct v4l2_plane __user *uplane;
>>  	compat_caddr_t p;
>> @@ -474,53 +584,60 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>>  	int ret;
>>  
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    put_user(kp->index, &up->index) ||
>> -	    put_user(kp->type, &up->type) ||
>> -	    put_user(kp->flags, &up->flags) ||
>> -	    put_user(kp->memory, &up->memory))
>> +	    convert_in_user(&up->index, &kp->index) ||
>> +	    get_user(type, &kp->type) ||
>> +	    put_user(type, &up->type) ||
>> +	    convert_in_user(&up->flags, &kp->flags) ||
>> +	    get_user(memory, &kp->memory) ||
>> +	    put_user(memory, &up->memory))
>>  		return -EFAULT;
>>  
>> -	if (put_user(kp->bytesused, &up->bytesused) ||
>> -	    put_user(kp->field, &up->field) ||
>> -	    put_user(kp->timestamp.tv_sec, &up->timestamp.tv_sec) ||
>> -	    put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>> -	    copy_to_user(&up->timecode, &kp->timecode, sizeof(kp->timecode)) ||
>> -	    put_user(kp->sequence, &up->sequence) ||
>> -	    put_user(kp->reserved2, &up->reserved2) ||
>> -	    put_user(kp->reserved, &up->reserved) ||
>> -	    put_user(kp->length, &up->length))
>> +	if (convert_in_user(&up->bytesused, &kp->bytesused) ||
>> +	    convert_in_user(&up->field, &kp->field) ||
>> +	    convert_in_user(&up->timestamp.tv_sec, &kp->timestamp.tv_sec) ||
>> +	    convert_in_user(&up->timestamp.tv_usec, &kp->timestamp.tv_usec) ||
>> +	    copy_in_user(&up->timecode, &kp->timecode, sizeof(kp->timecode)) ||
>> +	    convert_in_user(&up->sequence, &kp->sequence) ||
>> +	    convert_in_user(&up->reserved2, &kp->reserved2) ||
>> +	    convert_in_user(&up->reserved, &kp->reserved) ||
>> +	    get_user(length, &kp->length) ||
>> +	    put_user(length, &up->length))
>>  		return -EFAULT;
>>  
>> -	if (V4L2_TYPE_IS_MULTIPLANAR(kp->type)) {
>> -		num_planes = kp->length;
>> +	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
>> +		num_planes = length;
>>  		if (num_planes == 0)
>>  			return 0;
>>  
>> -		uplane = (__force struct v4l2_plane __user *)kp->m.planes;
>> +		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
>> +			return -EFAULT;
>>  		if (get_user(p, &up->m.planes))
>>  			return -EFAULT;
>>  		uplane32 = compat_ptr(p);
>>  
>>  		while (--num_planes >= 0) {
> 
> 
> while (num_planes--) {

Changed.

> 
> Or rather: s/num_planes/length/
> 
> Then you can remove num_planes.

But kept num_planes.

> 
>> -			ret = put_v4l2_plane32(uplane, uplane32, kp->memory);
>> +			ret = put_v4l2_plane32(uplane, uplane32, memory);
>>  			if (ret)
>>  				return ret;
>>  			++uplane;
>>  			++uplane32;
>>  		}
>>  	} else {
>> -		switch (kp->memory) {
>> +		switch (memory) {
>>  		case V4L2_MEMORY_MMAP:
>> -		case V4L2_MEMORY_OVERLAY:
>> -			if (put_user(kp->m.offset, &up->m.offset))
>> +			if (convert_in_user(&up->m.offset, &kp->m.offset))
>>  				return -EFAULT;
>>  			break;
>>  		case V4L2_MEMORY_USERPTR:
>> -			if (put_user(kp->m.userptr, &up->m.userptr))
>> +			if (convert_in_user(&up->m.userptr, &kp->m.userptr))
>> +				return -EFAULT;
>> +			break;
>> +		case V4L2_MEMORY_OVERLAY:
>> +			if (convert_in_user(&up->m.offset, &kp->m.offset))
> 
> Combine with MMAP case?

Changed.

> 
>>  				return -EFAULT;
>>  			break;
>>  		case V4L2_MEMORY_DMABUF:
>> -			if (put_user(kp->m.fd, &up->m.fd))
>> +			if (convert_in_user(&up->m.fd, &kp->m.fd))
>>  				return -EFAULT;
>>  			break;
>>  		}
>> @@ -545,29 +662,32 @@ struct v4l2_framebuffer32 {
>>  	} fmt;
>>  };
>>  
>> -static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_framebuffer32 __user *up)
>> +static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>> +				  struct v4l2_framebuffer32 __user *up)
>>  {
>> -	u32 tmp;
>> +	compat_caddr_t tmp;
>>  
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>>  	    get_user(tmp, &up->base) ||
>> -	    get_user(kp->capability, &up->capability) ||
>> -	    get_user(kp->flags, &up->flags) ||
>> -	    copy_from_user(&kp->fmt, &up->fmt, sizeof(up->fmt)))
>> +	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
>> +	    convert_in_user(&kp->capability, &up->capability) ||
>> +	    convert_in_user(&kp->flags, &up->flags) ||
>> +	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
>>  		return -EFAULT;
>> -	kp->base = (__force void *)compat_ptr(tmp);
>>  	return 0;
>>  }
>>  
>> -static int put_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_framebuffer32 __user *up)
>> +static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>> +				  struct v4l2_framebuffer32 __user *up)
>>  {
>> -	u32 tmp = (u32)((unsigned long)kp->base);
>> +	void *base;
>>  
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    put_user(tmp, &up->base) ||
>> -	    put_user(kp->capability, &up->capability) ||
>> -	    put_user(kp->flags, &up->flags) ||
>> -	    copy_to_user(&up->fmt, &kp->fmt, sizeof(up->fmt)))
>> +	    get_user(base, &kp->base) ||
>> +	    put_user(ptr_to_compat(base), &up->base) ||
>> +	    convert_in_user(&up->capability, &kp->capability) ||
>> +	    convert_in_user(&up->flags, &kp->flags) ||
>> +	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>> @@ -586,16 +706,18 @@ struct v4l2_input32 {
>>  
>>  /* The 64-bit v4l2_input struct has extra padding at the end of the struct.
>>     Otherwise it is identical to the 32-bit version. */
>> -static inline int get_v4l2_input32(struct v4l2_input *kp, struct v4l2_input32 __user *up)
>> +static inline int get_v4l2_input32(struct v4l2_input __user *kp,
>> +				   struct v4l2_input32 __user *up)
>>  {
>> -	if (copy_from_user(kp, up, sizeof(*up)))
>> +	if (copy_in_user(kp, up, sizeof(*up)))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>>  
>> -static inline int put_v4l2_input32(struct v4l2_input *kp, struct v4l2_input32 __user *up)
>> +static inline int put_v4l2_input32(struct v4l2_input __user *kp,
>> +				   struct v4l2_input32 __user *up)
>>  {
>> -	if (copy_to_user(up, kp, sizeof(*up)))
>> +	if (copy_in_user(up, kp, sizeof(*up)))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>> @@ -648,39 +770,58 @@ static inline bool ctrl_is_pointer(struct file *file, u32 id)
>>  	return !ret && (qec.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD);
>>  }
>>  
>> +static int bufsize_v4l2_ext_controls32(struct v4l2_ext_controls32 __user *up)
>> +{
>> +	u32 count;
>> +
>> +	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> +	    get_user(count, &up->count))
>> +		return -EFAULT;
>> +	if (count > V4L2_CID_MAX_CTRLS)
>> +		return -EINVAL;
>> +	return count * sizeof(struct v4l2_ext_control);
>> +}
>> +
>>  static int get_v4l2_ext_controls32(struct file *file,
>> -				   struct v4l2_ext_controls *kp,
>> -				   struct v4l2_ext_controls32 __user *up)
>> +				   struct v4l2_ext_controls __user *kp,
>> +				   struct v4l2_ext_controls32 __user *up,
>> +				   void __user *aux_buf, int aux_space)
>>  {
>>  	struct v4l2_ext_control32 __user *ucontrols;
>>  	struct v4l2_ext_control __user *kcontrols;
>> +	u32 count;
>>  	unsigned int n;
> 
> u32 n

Changed.

> 
>>  	compat_caddr_t p;
>>  
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> -	    get_user(kp->which, &up->which) ||
>> -	    get_user(kp->count, &up->count) ||
>> -	    get_user(kp->error_idx, &up->error_idx) ||
>> -	    copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>> +	    convert_in_user(&kp->which, &up->which) ||
>> +	    get_user(count, &up->count) ||
>> +	    put_user(count, &kp->count) ||
>> +	    convert_in_user(&kp->error_idx, &up->error_idx) ||
>> +	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>>  		return -EFAULT;
>> -	if (kp->count == 0) {
>> -		kp->controls = NULL;
>> -		return 0;
>> -	} else if (kp->count > V4L2_CID_MAX_CTRLS) {
>> +	if (count == 0)
>> +		return put_user(NULL, &kp->controls);
>> +	else if (kp->count > V4L2_CID_MAX_CTRLS)
> 
> count

Changed.

> 
>>  		return -EINVAL;
>> -	}
>>  	if (get_user(p, &up->controls))
>>  		return -EFAULT;
>>  	ucontrols = compat_ptr(p);
>> -	if (!access_ok(VERIFY_READ, ucontrols, kp->count * sizeof(*ucontrols)))
>> +	if (!access_ok(VERIFY_READ, ucontrols, count * sizeof(*ucontrols)))
>> +		return -EFAULT;
>> +	if (aux_space < count * sizeof(*kcontrols))
>>  		return -EFAULT;
>> -	kcontrols = compat_alloc_user_space(kp->count * sizeof(*kcontrols));
>> -	kp->controls = (__force struct v4l2_ext_control *)kcontrols;
>> -	for (n = 0; n < kp->count; n++) {
>> +	kcontrols = aux_buf;
>> +	if (put_user((__force struct v4l2_ext_control *)kcontrols,
>> +		     &kp->controls))
>> +		return -EFAULT;
>> +
>> +	for (n = 0; n < count; n++) {
>>  		u32 id;
>>  
>>  		if (copy_in_user(kcontrols, ucontrols, sizeof(*ucontrols)))
>>  			return -EFAULT;
>> +
>>  		if (get_user(id, &kcontrols->id))
>>  			return -EFAULT;
> 
> Newline here, too? Not really a problem with this patch though.

Changed.

> 
>>  		if (ctrl_is_pointer(file, id)) {
>> @@ -699,36 +840,47 @@ static int get_v4l2_ext_controls32(struct file *file,
>>  }
>>  
>>  static int put_v4l2_ext_controls32(struct file *file,
>> -				   struct v4l2_ext_controls *kp,
>> +				   struct v4l2_ext_controls __user *kp,
>>  				   struct v4l2_ext_controls32 __user *up)
>>  {
>>  	struct v4l2_ext_control32 __user *ucontrols;
>> -	struct v4l2_ext_control __user *kcontrols =
>> -		(__force struct v4l2_ext_control __user *)kp->controls;
>> -	int n = kp->count;
>> +	struct v4l2_ext_control __user *kcontrols;
>> +	u32 count;
>> +	unsigned int n;
> 
> u32 n

Changed.

> 
>>  	compat_caddr_t p;
>>  
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    put_user(kp->which, &up->which) ||
>> -	    put_user(kp->count, &up->count) ||
>> -	    put_user(kp->error_idx, &up->error_idx) ||
>> -	    copy_to_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>> +	    get_user(kcontrols, &kp->controls) ||
> 
> How about doing this as last? "controls" is the last member of the struct.

Changed.

> 
>> +	    convert_in_user(&up->which, &kp->which) ||
>> +	    get_user(count, &kp->count) ||
>> +	    put_user(count, &up->count) ||
>> +	    convert_in_user(&up->error_idx, &kp->error_idx) ||
>> +	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>>  		return -EFAULT;
>> -	if (!kp->count)
>> +	if (!count)
>>  		return 0;
>>  
>>  	if (get_user(p, &up->controls))
>>  		return -EFAULT;
>>  	ucontrols = compat_ptr(p);
>> -	if (!access_ok(VERIFY_WRITE, ucontrols, n * sizeof(*ucontrols)))
>> +	if (!access_ok(VERIFY_WRITE, ucontrols, count * sizeof(*ucontrols)))
>>  		return -EFAULT;
>>  
>> -	while (--n >= 0) {
>> -		unsigned size = sizeof(*ucontrols);
>> +	for (n = 0; n < count; n++) {
>> +		unsigned int size = sizeof(*ucontrols);
>>  		u32 id;
>>  
>> +		if (copy_in_user(&ucontrols->id, &kcontrols->id,
>> +				 sizeof(ucontrols->id)) ||
> 
> get_user(id, &kcontrols->id) ||
> put_user(id, &ucontrols->id) ||

Changed.

> 
>> +		    copy_in_user(&ucontrols->size, &kcontrols->size,
>> +				 sizeof(ucontrols->size)) ||
> 
> convert_in_user() ?

Changed.

> 
>> +		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
>> +				 sizeof(ucontrols->reserved2)))
>> +			return -EFAULT;
>> +
>>  		if (get_user(id, &kcontrols->id))
>>  			return -EFAULT;
> 
> ...and the above if () can be removed.

Done.

> 
>> +
>>  		/* Do not modify the pointer when copying a pointer control.
>>  		   The contents of the pointer was changed, not the pointer
>>  		   itself. */
>> @@ -736,6 +888,7 @@ static int put_v4l2_ext_controls32(struct file *file,
>>  			size -= sizeof(ucontrols->value64);
> 
> This chunk is yearning for newlines. Not a fault of this patch though.
> 
>>  		if (copy_in_user(ucontrols, kcontrols, size))
>>  			return -EFAULT;
>> +
>>  		ucontrols++;
>>  		kcontrols++;
>>  	}
>> @@ -755,17 +908,18 @@ struct v4l2_event32 {
>>  	__u32				reserved[8];
>>  };
>>  
>> -static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32 __user *up)
>> +static int put_v4l2_event32(struct v4l2_event __user *kp,
>> +			    struct v4l2_event32 __user *up)
>>  {
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    put_user(kp->type, &up->type) ||
>> -	    copy_to_user(&up->u, &kp->u, sizeof(kp->u)) ||
>> -	    put_user(kp->pending, &up->pending) ||
>> -	    put_user(kp->sequence, &up->sequence) ||
>> -	    put_user(kp->timestamp.tv_sec, &up->timestamp.tv_sec) ||
>> -	    put_user(kp->timestamp.tv_nsec, &up->timestamp.tv_nsec) ||
>> -	    put_user(kp->id, &up->id) ||
>> -	    copy_to_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
>> +	    convert_in_user(&up->type, &kp->type) ||
>> +	    copy_in_user(&up->u, &kp->u, sizeof(kp->u)) ||
>> +	    convert_in_user(&up->pending, &kp->pending) ||
>> +	    convert_in_user(&up->sequence, &kp->sequence) ||
>> +	    convert_in_user(&up->timestamp.tv_sec, &kp->timestamp.tv_sec) ||
>> +	    convert_in_user(&up->timestamp.tv_nsec, &kp->timestamp.tv_nsec) ||
>> +	    convert_in_user(&up->id, &kp->id) ||
>> +	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>> @@ -778,31 +932,34 @@ struct v4l2_edid32 {
>>  	compat_caddr_t edid;
>>  };
>>  
>> -static int get_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
>> +static int get_v4l2_edid32(struct v4l2_edid __user *kp,
>> +			   struct v4l2_edid32 __user *up)
>>  {
>> -	u32 tmp;
>> +	compat_uptr_t tmp;
>>  
>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>> -	    get_user(kp->pad, &up->pad) ||
>> -	    get_user(kp->start_block, &up->start_block) ||
>> -	    get_user(kp->blocks, &up->blocks) ||
>> +	    convert_in_user(&kp->pad, &up->pad) ||
>> +	    convert_in_user(&kp->start_block, &up->start_block) ||
>> +	    convert_in_user(&kp->blocks, &up->blocks) ||
>>  	    get_user(tmp, &up->edid) ||
>> -	    copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>> +	    put_user(compat_ptr(tmp), &kp->edid) ||
>> +	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>>  		return -EFAULT;
>> -	kp->edid = (__force u8 *)compat_ptr(tmp);
>>  	return 0;
>>  }
>>  
>> -static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
>> +static int put_v4l2_edid32(struct v4l2_edid __user *kp,
>> +			   struct v4l2_edid32 __user *up)
>>  {
>> -	u32 tmp = (u32)((unsigned long)kp->edid);
>> +	void *edid;
>>  
>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>> -	    put_user(kp->pad, &up->pad) ||
>> -	    put_user(kp->start_block, &up->start_block) ||
>> -	    put_user(kp->blocks, &up->blocks) ||
>> -	    put_user(tmp, &up->edid) ||
>> -	    copy_to_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>> +	    convert_in_user(&up->pad, &kp->pad) ||
>> +	    convert_in_user(&up->start_block, &kp->start_block) ||
>> +	    convert_in_user(&up->blocks, &kp->blocks) ||
>> +	    get_user(edid, &kp->edid) ||
>> +	    put_user(ptr_to_compat(edid), &up->edid) ||
>> +	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>>  		return -EFAULT;
>>  	return 0;
>>  }
>> @@ -835,22 +992,30 @@ static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up)
>>  #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
>>  #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
>>  
>> +static void __user *alloc_userspace(unsigned int size, int aux_space, long *err)
> 
> This one is weird. Rather than following the pattern and returning an error
> and assigning the result to a pointer passed to it, it does exactly the
> opposite. I'd change that. I guess this is a matter of opinion to some
> degree at least, though.

Changed. It much nicer now.

> 
>> +{
>> +	void __user *up_native;
>> +
>> +	if (aux_space < 0) {
>> +		*err = aux_space;
>> +		return NULL;
>> +	}
>> +	up_native = compat_alloc_user_space(size + aux_space);
>> +	if (!up_native)
>> +		*err = -ENOMEM;
>> +	else if (clear_user(up_native, size))
>> +		*err = -EFAULT;
>> +	else
>> +		return up_native;
>> +	return NULL;
>> +}
>> +
>>  static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>  {
>> -	union {
>> -		struct v4l2_format v2f;
>> -		struct v4l2_buffer v2b;
>> -		struct v4l2_framebuffer v2fb;
>> -		struct v4l2_input v2i;
>> -		struct v4l2_standard v2s;
>> -		struct v4l2_ext_controls v2ecs;
>> -		struct v4l2_event v2ev;
>> -		struct v4l2_create_buffers v2crt;
>> -		struct v4l2_edid v2edid;
>> -		unsigned long vx;
>> -		int vi;
>> -	} karg;
>>  	void __user *up = compat_ptr(arg);
>> +	void __user *up_native = NULL;
>> +	void __user *aux_buf;
>> +	int aux_space;
> 
> size_t?

I went with u32 :-)

> 
>>  	int compatible_arg = 1;
>>  	long err = 0;
>>  
>> @@ -889,30 +1054,47 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	case VIDIOC_STREAMOFF:
>>  	case VIDIOC_S_INPUT:
>>  	case VIDIOC_S_OUTPUT:
>> -		err = get_user(karg.vi, (s32 __user *)up);
>> +		up_native = alloc_userspace(sizeof(unsigned int __user),
>> +					    0, &err);
>> +		if (!err && convert_in_user((unsigned int __user *)up_native,
>> +					    (compat_uint_t __user *)up))
>> +			return -EFAULT;
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_INPUT:
>>  	case VIDIOC_G_OUTPUT:
>> +		up_native = alloc_userspace(sizeof(unsigned int __user),
>> +					    0, &err);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_EDID:
>>  	case VIDIOC_S_EDID:
>> -		err = get_v4l2_edid32(&karg.v2edid, up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_edid), 0, &err);
>> +		err = err ? : get_v4l2_edid32(up_native, up);
> 
> I'd write this as:
> 
> 		if (!err)
> 			err = get_v4l2_edid32(native_up, up);
> 
> Same below.

Done.

> 
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_FMT:
>>  	case VIDIOC_S_FMT:
>>  	case VIDIOC_TRY_FMT:
>> -		err = get_v4l2_format32(&karg.v2f, up);
>> +		aux_space = bufsize_v4l2_format32(up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_format),
>> +					    aux_space, &err);
>> +		aux_buf = up_native + sizeof(struct v4l2_format);
>> +		err = err ? : get_v4l2_format32(up_native, up,
>> +						aux_buf, aux_space);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_CREATE_BUFS:
>> -		err = get_v4l2_create32(&karg.v2crt, up);
>> +		aux_space = bufsize_v4l2_create32(up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_create_buffers),
>> +					    aux_space, &err);
>> +		aux_buf = up_native + sizeof(struct v4l2_create_buffers);
>> +		err = err ? : get_v4l2_create32(up_native, up,
>> +						aux_buf, aux_space);
>>  		compatible_arg = 0;
>>  		break;
>>  
>> @@ -920,36 +1102,54 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	case VIDIOC_QUERYBUF:
>>  	case VIDIOC_QBUF:
>>  	case VIDIOC_DQBUF:
>> -		err = get_v4l2_buffer32(&karg.v2b, up);
>> +		aux_space = bufsize_v4l2_buffer32(up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_buffer),
>> +					    aux_space, &err);
>> +		aux_buf = up_native + sizeof(struct v4l2_buffer);
>> +		err = err ? : get_v4l2_buffer32(up_native, up,
>> +						aux_buf, aux_space);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_S_FBUF:
>> -		err = get_v4l2_framebuffer32(&karg.v2fb, up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_framebuffer),
>> +					    0, &err);
>> +		err = err ? : get_v4l2_framebuffer32(up_native, up);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_FBUF:
>> +		up_native = alloc_userspace(sizeof(struct v4l2_framebuffer),
>> +					    0, &err);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_ENUMSTD:
>> -		err = get_v4l2_standard32(&karg.v2s, up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_standard),
>> +					    0, &err);
>> +		err = err ? : get_v4l2_standard32(up_native, up);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_ENUMINPUT:
>> -		err = get_v4l2_input32(&karg.v2i, up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_input), 0, &err);
>> +		err = err ? : get_v4l2_input32(up_native, up);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_EXT_CTRLS:
>>  	case VIDIOC_S_EXT_CTRLS:
>>  	case VIDIOC_TRY_EXT_CTRLS:
>> -		err = get_v4l2_ext_controls32(file, &karg.v2ecs, up);
>> +		aux_space = bufsize_v4l2_ext_controls32(up);
>> +		up_native = alloc_userspace(sizeof(struct v4l2_ext_controls),
>> +					    aux_space, &err);
>> +		aux_buf = up_native + sizeof(struct v4l2_ext_controls);
>> +		err = err ? : get_v4l2_ext_controls32(file, up_native, up,
>> +						      aux_buf, aux_space);
>>  		compatible_arg = 0;
>>  		break;
>>  	case VIDIOC_DQEVENT:
>> +		up_native = alloc_userspace(sizeof(struct v4l2_event), 0, &err);
>>  		compatible_arg = 0;
>>  		break;
>>  	}
>> @@ -958,13 +1158,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  
>>  	if (compatible_arg)
>>  		err = native_ioctl(file, cmd, (unsigned long)up);
>> -	else {
>> -		mm_segment_t old_fs = get_fs();
>> -
>> -		set_fs(KERNEL_DS);
>> -		err = native_ioctl(file, cmd, (unsigned long)&karg);
>> -		set_fs(old_fs);
>> -	}
>> +	else
>> +		err = native_ioctl(file, cmd, (unsigned long)up_native);
>>  
>>  	if (err == -ENOTTY || err == -EFAULT || err == -ENOIOCTLCMD)
>>  		return err;
>> @@ -976,11 +1171,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	case VIDIOC_G_EXT_CTRLS:
>>  	case VIDIOC_S_EXT_CTRLS:
>>  	case VIDIOC_TRY_EXT_CTRLS:
>> -		if (put_v4l2_ext_controls32(file, &karg.v2ecs, up))
>> +		if (put_v4l2_ext_controls32(file, up_native, up))
>>  			err = -EFAULT;
>>  		break;
>>  	case VIDIOC_S_EDID:
>> -		if (put_v4l2_edid32(&karg.v2edid, up))
>> +		if (put_v4l2_edid32(up_native, up))
>>  			err = -EFAULT;
>>  		break;
>>  	}
>> @@ -992,44 +1187,45 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	case VIDIOC_S_OUTPUT:
>>  	case VIDIOC_G_INPUT:
>>  	case VIDIOC_G_OUTPUT:
>> -		err = put_user(((s32)karg.vi), (s32 __user *)up);
>> +		err = convert_in_user((compat_uint_t __user *)up,
>> +				      ((unsigned int __user *)up_native));
> 
> Do note that convert_in_user() returns Boolean. This will effectively yield
> 1 in case of an error here. How about changing convert_in_user() to return
> whatever get_user / put_user do, to avoid errors in the future?

As mentioned at the top I decided not to change convert_in_user (now assign_in_user)
and modified this code here instead.

> 
>>  		break;
>>  
>>  	case VIDIOC_G_FBUF:
>> -		err = put_v4l2_framebuffer32(&karg.v2fb, up);
>> +		err = put_v4l2_framebuffer32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_DQEVENT:
>> -		err = put_v4l2_event32(&karg.v2ev, up);
>> +		err = put_v4l2_event32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_G_EDID:
>> -		err = put_v4l2_edid32(&karg.v2edid, up);
>> +		err = put_v4l2_edid32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_G_FMT:
>>  	case VIDIOC_S_FMT:
>>  	case VIDIOC_TRY_FMT:
>> -		err = put_v4l2_format32(&karg.v2f, up);
>> +		err = put_v4l2_format32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_CREATE_BUFS:
>> -		err = put_v4l2_create32(&karg.v2crt, up);
>> +		err = put_v4l2_create32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_PREPARE_BUF:
>>  	case VIDIOC_QUERYBUF:
>>  	case VIDIOC_QBUF:
>>  	case VIDIOC_DQBUF:
>> -		err = put_v4l2_buffer32(&karg.v2b, up);
>> +		err = put_v4l2_buffer32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_ENUMSTD:
>> -		err = put_v4l2_standard32(&karg.v2s, up);
>> +		err = put_v4l2_standard32(up_native, up);
>>  		break;
>>  
>>  	case VIDIOC_ENUMINPUT:
>> -		err = put_v4l2_input32(&karg.v2i, up);
>> +		err = put_v4l2_input32(up_native, up);
>>  		break;
>>  	}
>>  	return err;
> 

Regards,

	Hans