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

2018-04-16 Thread Mauro Carvalho Chehab 
> >>> @@ -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, >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, >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 *)>id)
> > ...
> > 
> > In this specific case, only copy_in_user() was missing it; all the other
> > __user casts are there already.
> > 
> >   
> >> Note: the whole 'u' and 'k' naming is now hopelessly out of date 
> >> and
> >> confusing. It should really be '32' and '64' to denote 32 bit vs
> >> 64 bit layout. The pointers are now always in userspace, so 'k' 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(>start_block, >start_block) ||
> >>>   assign_in_user(>blocks, >blocks) ||
> >>>   get_user(tmp, >edid) ||
> >>> - put_user(compat_ptr(tmp), >edid) ||
> >>> + put_user((void __force *)compat_ptr(tmp), >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(>start_block, >start_block) ||
> >>>   assign_in_user(>blocks, >blocks) ||
> >>>   get_user(edid, >edid) ||
> >>> - put_user(ptr_to_compat(edid), >edid) ||
> >>> + put_user(ptr_to_compat((void __user *)edid), >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

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

2018-04-16 Thread Hans Verkuil 
On 04/16/2018 04:50 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 16 Apr 2018 14:03:45 +0200
> Hans Verkuil  escreveu:
> 
>> On 04/13/2018 08:07 PM, 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] *uptr
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:got void 
>>> *
>>>   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] *
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:got struct 
>>> v4l2_plane [noderef] **
>>>   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] *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] *kcontrols
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:got struct 
>>> v4l2_ext_control *
>>>   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] *
>>>   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] *uptr
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:got void 
>>> *[assigned] edid
>>>
>>> Fix them.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab 
>>> ---
>>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 
>>> ++-
>>>  1 file changed, 17 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
>>> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>>> index d03a44d89649..0b9dfe7dbfe7 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, >m.userptr) ||
>>> -   put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
>>> +   if (get_user(p, >m.userptr)||
>>> +   put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>>>  >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;  
>>
>> This needs a comment (either here or before the get_user below). It really 
>> is a
>> pointer to userspace, but since videodev2.h has it without __user (since it 
>> is
>> copied to kernel space in v4l2-ioctl.c) we need to define it as a regular 
>> pointer
>> here and cast it to a __user pointer in the put_v4l2_plane32() call.
>>
>> This is not trivially obvious, so a comment would help a lot.
> 
> 
> 
>>
>>> compat_caddr_t p;
>>> int ret;
>>>  
>>> @@ -617,15 +617,14 @@ 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 
>>> **)>m.planes)))
>>> +   if (get_user(uplane, >m.planes))
>>> return -EFAULT;
>>> if (get_user(p, >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 +674,7 @@ static int get_v4l2_framebuffer32(struct 
>>> v4l2_framebuffer __user *kp,
>>>  
>>> if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>>>

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

2018-04-16 Thread Mauro Carvalho Chehab 
Em Mon, 16 Apr 2018 14:03:45 +0200
Hans Verkuil  escreveu:

> On 04/13/2018 08:07 PM, 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] *uptr
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:got void 
> > *
> >   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] *
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:got struct 
> > v4l2_plane [noderef] **
> >   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] *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] *kcontrols
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:got struct 
> > v4l2_ext_control *
> >   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] *
> >   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] *uptr
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:got void 
> > *[assigned] edid
> > 
> > Fix them.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 
> > ++-
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
> > b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index d03a44d89649..0b9dfe7dbfe7 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, >m.userptr) ||
> > -   put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> > +   if (get_user(p, >m.userptr)||
> > +   put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
> >  >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;  
> 
> This needs a comment (either here or before the get_user below). It really is 
> a
> pointer to userspace, but since videodev2.h has it without __user (since it is
> copied to kernel space in v4l2-ioctl.c) we need to define it as a regular 
> pointer
> here and cast it to a __user pointer in the put_v4l2_plane32() call.
> 
> This is not trivially obvious, so a comment would help a lot.



> 
> > compat_caddr_t p;
> > int ret;
> >  
> > @@ -617,15 +617,14 @@ 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 
> > **)>m.planes)))
> > +   if (get_user(uplane, >m.planes))
> > return -EFAULT;
> > if (get_user(p, >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 +674,7 @@ static int get_v4l2_framebuffer32(struct 
> > v4l2_framebuffer __user *kp,
> >  
> > if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> > get_user(tmp, >base) ||
> > -   put_user((__force void

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

2018-04-16 Thread Hans Verkuil 
On 04/13/2018 08:07 PM, 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] *uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:got void *
>   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] *
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:got struct 
> v4l2_plane [noderef] **
>   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] *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] *kcontrols
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:got struct 
> v4l2_ext_control *
>   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] 
> *
>   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] *uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:got void 
> *[assigned] edid
> 
> Fix them.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 
> ++-
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d03a44d89649..0b9dfe7dbfe7 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, >m.userptr) ||
> - put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> + if (get_user(p, >m.userptr)||
> + put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>>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;

This needs a comment (either here or before the get_user below). It really is a
pointer to userspace, but since videodev2.h has it without __user (since it is
copied to kernel space in v4l2-ioctl.c) we need to define it as a regular 
pointer
here and cast it to a __user pointer in the put_v4l2_plane32() call.

This is not trivially obvious, so a comment would help a lot.

>   compat_caddr_t p;
>   int ret;
>  
> @@ -617,15 +617,14 @@ 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 
> **)>m.planes)))
> + if (get_user(uplane, >m.planes))
>   return -EFAULT;
>   if (get_user(p, >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 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer 
> __user *kp,
>  
>   if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>   get_user(tmp, >base) ||
> - put_user((__force void *)compat_ptr(tmp), >base) ||
> + put_user((void __force *)compat_ptr(tmp), >base) ||
>   assign_in_user(>capability, >capability) ||
>   assign_in_user(>flags, >flags) ||
>   copy_in_user(>fmt, >fmt, sizeof(kp->fmt)))
> @@