Re: [PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

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

> >>> @@ -898,7 +899,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))    
> >>
> >> This is rather ugly. Would it be better to do something like this:
> >>
> >> 	struct v4l2_ext_control __user *kcontrols
> >> 	struct v4l2_ext_control *kcontrols_tmp;
> >>
> >> 	get_user(kcontrols_tmp, &kp->controls);
> >> 	kcontrols = (void __user __force *)kcontrols_tmp;  
> > 
> > Nah, having two pointers with the same value, one with __user and another
> > one without it will make it really hard for people to review.
> >   
> >>
> >> And then there is no need to change anything else.
> >>
> >> Regardless of the chosen solution, this needs comments to explain what is going
> >> on here, just as with v4l2_buffer above.  
> > 
> > Actually, the problem here happens before that. The problem
> > here (and on the previous one) is that get_user() expects that
> > the second argument to be a Kernelspace pointer.  
> 
> You mean the first argument? I'm actually not entirely sure if this is
> correct since __get_user calls __typeof__, but that might not know about
> __user. Anyway, it does not really matter for this patch and the uaccess.h
> code gives me a headache :-)

__typeof__ gets the type of the var, but __user is not a type. it is
something else (see include/linux/compiler_types.h):

	#ifdef __CHECKER__
	# define __user		__attribute__((noderef, address_space(1)))
	...
	#else /* __CHECKER__ */
	# ifdef STRUCTLEAK_PLUGIN
	#  define __user __attribute__((user))
	# else
	#  define __user
	# endif
	...
	#endif /* __CHECKER__ */

It is only built for sparse/smatch and uses __attribute__ definition,
with I guess __type_of__() won't parse.

Also, on almost all places where get_user() is called, what you expect
is to fill a Kernelspace var with something that came from userspace.

So, it expects that the first argument to be a pointer to an area
that is at Kernelspace.

> 
> > 
> > So, in this routine, it does (simplified):
> > 
> >         struct v4l2_ext_control32 __user *ucontrols;
> >         struct v4l2_ext_control *kcontrols;
> > 
> > ...
> > 	get_user(kcontrols, &kp->controls);
> > 
> > Then, every time it needs to get something related to it,
> > it needs the __user, like here:
> > 
> > 	if (get_user(id, (unsigned int __user *)&kcontrols->id)
> > 	...
> > 
> > In this specific case, only copy_in_user() was missing it; all the other
> > __user casts are there already.
> > 
> >   
> >> Note: the whole 'u<foo>' and 'k<foo>' naming is now hopelessly out of date and
> >> confusing. It should really be '<foo>32' and '<foo>64' to denote 32 bit vs
> >> 64 bit layout. The pointers are now always in userspace, so 'k<foo>' no longer
> >> makes sense.  
> > 
> > Yes, we need this change, but this should be at a separate patch.
> > 
> > I can do it, after we cleanup v4l2-compat-ioctl32.c from their namespace
> > mess.  
> 
> Of course, this is a separate patch.
> 
> > 
> >   
> >>  
> >>>  			return -EFAULT;
> >>>  
> >>>  		ucontrols++;
> >>> @@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
> >>>  	    assign_in_user(&kp->start_block, &up->start_block) ||
> >>>  	    assign_in_user(&kp->blocks, &up->blocks) ||
> >>>  	    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 +972,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;
> >>>     
> >>
> >> Otherwise this patch looks good.
> >>
> >> Regards,
> >>
> >> 	Hans  
> > 
> > Would be ok if I fold (or add as a separate patch) the enclosed diff?
> > 
> > Thanks,
> > Mauro
> > 
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index 1057ab8ce2b6..5c3408bdfd89 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -617,6 +617,15 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >  
> >  		if (num_planes == 0)
> >  			return 0;
> > +		/*
> > +		 * We needed to define uplane without user.
> > +		 * The reason is that v4l2-ioctl.c copies it from userspace
> > +		 * into Kernelspace, so it's definition at videodev2.h doesn't
> > +		 * have an __user markup. That makes get_user() to do wrong
> > +		 * casts, as pointed by smatch.
> > +		 * So, instead, declare it as ks, and pass it as an userspace
> > +		 * pointer to put_v4l2_plane32().
> > +		 */  
> 
> I would propose this text:
> 
> "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()."
> 
> (yes, it's 'a user'. See https://ell.stackexchange.com/questions/26986/why-a-user-instead-of-an-user)
> 
> >  		if (get_user(uplane, &kp->m.planes))
> >  			return -EFAULT;
> >  		if (get_user(p, &up->m.planes))
> > @@ -861,6 +870,15 @@ static int put_v4l2_ext_controls32(struct file *file,
> >  	u32 n;
> >  	compat_caddr_t p;
> >  
> > +	/*
> > +	 * We needed to define kcontrols without user.
> > +	 * The reason is that v4l2-ioctl.c copies it from userspace
> > +	 * into Kernelspace, so it's definition at videodev2.h doesn't
> > +	 * have an __user markup. That makes get_user() & friends to do
> > +	 * wrong casts, as pointed by smatch.
> > +	 * So, instead, declare it as ks, and pass it as an userspace
> > +	 * pointer where needed.
> > +	 */  
> 
> "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."

Works for me. I'll send patch 17/17 as a separate series, with the above
merged on the first patch.

Regards,
Mauro