| Message ID | 510d0652872c612db21be8b846755f80e3cc4588.1523960171.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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 *. > 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. > 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
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. > > > 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. (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. > > > 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
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. > >> >>> 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). > > (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. 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 >
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
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) || 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) || + copy_in_user(&ucontrols->reserved2, + (unsigned int __user *)&kcontrols->reserved2, 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)) 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) || 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;
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(-)