Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Tue, 17 Apr 2018 13:04:31 +0200
Hans Verkuil <hansverk@cisco.com> escreveu:

> On 04/17/18 12:53, Mauro Carvalho Chehab wrote:
> > Em Tue, 17 Apr 2018 12:33:11 +0200
> > Hans Verkuil <hansverk@cisco.com> escreveu:
> >   
> >> On 04/17/18 12:20, Mauro Carvalho Chehab wrote:  
> >>> Smatch report several issues with bad __user annotations:
> >>>
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
> >>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid
> >>>
> >>> Fix them.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
> >>>  1 file changed, 35 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >>> index d03a44d89649..c951ac3faf46 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >>> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
> >>>  			return -EFAULT;
> >>>  		break;
> >>>  	case V4L2_MEMORY_USERPTR:
> >>> -		if (get_user(p, &up->m.userptr) ||
> >>> -		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> >>> +		if (get_user(p, &up->m.userptr)||
> >>> +		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
> >>>  			     &up32->m.userptr))
> >>>  			return -EFAULT;
> >>>  		break;
> >>> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >>>  	u32 length;
> >>>  	enum v4l2_memory memory;
> >>>  	struct v4l2_plane32 __user *uplane32;
> >>> -	struct v4l2_plane __user *uplane;
> >>> +	struct v4l2_plane *uplane;
> >>>  	compat_caddr_t p;
> >>>  	int ret;
> >>>  
> >>> @@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >>>  
> >>>  		if (num_planes == 0)
> >>>  			return 0;
> >>> -
> >>> -		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
> >>> +		/* We need to define uplane without __user, even though
> >>> +		 * it does point to data in userspace here. The reason is
> >>> +		 * that v4l2-ioctl.c copies it from userspace to kernelspace,
> >>> +		 * so its definition in videodev2.h doesn't have a
> >>> +		 * __user markup. Defining uplane with __user causes
> >>> +		 * smatch warnings, so instead declare it without __user
> >>> +		 * and cast it as a userspace pointer to put_v4l2_plane32().
> >>> +		 */
> >>> +		if (get_user(uplane, &kp->m.planes))
> >>>  			return -EFAULT;
> >>>  		if (get_user(p, &up->m.planes))
> >>>  			return -EFAULT;
> >>>  		uplane32 = compat_ptr(p);
> >>>  
> >>>  		while (num_planes--) {
> >>> -			ret = put_v4l2_plane32(uplane, uplane32, memory);
> >>> +			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
> >>>  			if (ret)
> >>>  				return ret;
> >>>  			++uplane;
> >>> @@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
> >>>  
> >>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >>>  	    get_user(tmp, &up->base) ||
> >>> -	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> >>> +	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
> >>>  	    assign_in_user(&kp->capability, &up->capability) ||
> >>>  	    assign_in_user(&kp->flags, &up->flags) ||
> >>>  	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
> >>> @@ -690,7 +697,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
> >>>  
> >>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >>>  	    get_user(base, &kp->base) ||
> >>> -	    put_user(ptr_to_compat(base), &up->base) ||
> >>> +	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
> >>>  	    assign_in_user(&up->capability, &kp->capability) ||
> >>>  	    assign_in_user(&up->flags, &kp->flags) ||
> >>>  	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
> >>> @@ -857,11 +864,19 @@ static int put_v4l2_ext_controls32(struct file *file,
> >>>  				   struct v4l2_ext_controls32 __user *up)
> >>>  {
> >>>  	struct v4l2_ext_control32 __user *ucontrols;
> >>> -	struct v4l2_ext_control __user *kcontrols;
> >>> +	struct v4l2_ext_control *kcontrols;
> >>>  	u32 count;
> >>>  	u32 n;
> >>>  	compat_caddr_t p;
> >>>  
> >>> +	/*
> >>> +	 * We need to define kcontrols without __user, even though it does
> >>> +	 * point to data in userspace here. The reason is that v4l2-ioctl.c
> >>> +	 * copies it from userspace to kernelspace, so its definition in
> >>> +	 * videodev2.h doesn't have a __user markup. Defining kcontrols
> >>> +	 * with __user causes smatch warnings, so instead declare it
> >>> +	 * without __user and cast it as a userspace pointer where needed.
> >>> +	 */
> >>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >>>  	    assign_in_user(&up->which, &kp->which) ||
> >>>  	    get_user(count, &kp->count) ||
> >>> @@ -883,10 +898,12 @@ static int put_v4l2_ext_controls32(struct file *file,
> >>>  		unsigned int size = sizeof(*ucontrols);
> >>>  		u32 id;
> >>>  
> >>> -		if (get_user(id, &kcontrols->id) ||
> >>> +		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||    
> >>
> >> Why use 'unsigned int' instead of u32? It's defined as __u32 in the header,
> >> so let's keep this consistent.  
> > 
> > Makes sense.
> > 
> > It should be noticed, however, that, on all other similar casts that are
> > already there, it uses unsigned int:
> > 
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:          unsigned int size = sizeof(*ucontrols);
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:          err = alloc_userspace(sizeof(unsigned int), 0, &up_native);
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:          if (!err && assign_in_user((unsigned int __user *)up_native,
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:          err = alloc_userspace(sizeof(unsigned int), 0, &up_native);
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:                             ((unsigned int __user *)up_native)))
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
> > 
> > So, I tried to be consistent with that.  
> 
> Those casts are probably very old and they should be fixed at some point.

Sure, but, IMHO, the best is to do it on a separate patch.

> 
> >   
> >>  
> >>>  		    put_user(id, &ucontrols->id) ||
> >>> -		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
> >>> -		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
> >>> +		    assign_in_user(&ucontrols->size,
> >>> +				   (unsigned int __user *)&kcontrols->size) ||    
> >>
> >> Same here.
> >>  
> >>> +		    copy_in_user(&ucontrols->reserved2,
> >>> +				 (unsigned int __user *)&kcontrols->reserved2,    
> >>
> >> This can be a void __user *.  
> > 
> > We should be very careful changing it to void. When I tested the first
> > version of this patchset, I noticed that the results produced by one
> > ioctl were different with v4l2-compliance, between 32/64 bits version,
> > because the type of the cast was wrong.
> > 
> > So, it should really match the type of the fields that will be copying,
> > as otherwise we may have troubles.  
> 
> copy_in_user has void * arguments, and it is just a memcpy effectively.
> There is no point in sticking to the field types (and it doesn't do that
> either in this code).

I don't remember what routine caused compat32 issues, but I guess it was
a get_user/put_user/assign_in_user. There, the type is important.

Yeah, copy_in_user() can safely use void *. I'll switch all occurrences
with casts to void *.

> 
> > 
> > (same applies to your similar comments below)
> >   
> >>  
> >>>  				 sizeof(ucontrols->reserved2)))
> >>>  			return -EFAULT;
> >>>  
> >>> @@ -898,7 +915,8 @@ static int put_v4l2_ext_controls32(struct file *file,
> >>>  		if (ctrl_is_pointer(file, id))
> >>>  			size -= sizeof(ucontrols->value64);
> >>>  
> >>> -		if (copy_in_user(ucontrols, kcontrols, size))
> >>> +		if (copy_in_user(ucontrols,
> >>> +			         (unsigned int __user *)kcontrols, size))    
> >>
> >> void __user *
> >>  
> >>>  			return -EFAULT;
> >>>  
> >>>  		ucontrols++;
> >>> @@ -952,9 +970,10 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
> >>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >>>  	    assign_in_user(&kp->pad, &up->pad) ||
> >>>  	    assign_in_user(&kp->start_block, &up->start_block) ||
> >>> -	    assign_in_user(&kp->blocks, &up->blocks) ||
> >>> +	    assign_in_user(&kp->blocks,
> >>> +			   (unsigned char __user *)&up->blocks) ||    
> >>  
> >> ->blocks is a u32, so this should be a u32 cast as well.    
> 
> Be aware that the unsigned char * cast is actually a bug: it will clamp the
> u32 'blocks' value to a u8.

True.

I'll write a separate patch addressing the cast types and will run some
tests to be sure that v4l2-compliance won't produce any error.

The above issue, however (u8 instead of u32) is something that I suspect
that the compliance tool won't catch. So, I won't doubt that we have
some hidden bugs at the compat code.

> 
> Regards,
> 
> 	Hans
> 
> >>  
> >>>  	    get_user(tmp, &up->edid) ||
> >>> -	    put_user(compat_ptr(tmp), &kp->edid) ||
> >>> +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
> >>>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >>>  		return -EFAULT;
> >>>  	return 0;
> >>> @@ -970,7 +989,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
> >>>  	    assign_in_user(&up->start_block, &kp->start_block) ||
> >>>  	    assign_in_user(&up->blocks, &kp->blocks) ||
> >>>  	    get_user(edid, &kp->edid) ||
> >>> -	    put_user(ptr_to_compat(edid), &up->edid) ||
> >>> +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
> >>>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >>>  		return -EFAULT;
> >>>  	return 0;
> >>>     
> >>
> >> Regards,
> >>
> >> 	Hans  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro