| Message ID | 26ee885cc6b9581e6d4433e4248f8611f4c007c4.1524136402.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/19/18 13:15, 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) || > 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) || This must be a (u32 __user *) cast, otherwise this patch will break EDID handling on big-endian systems. I know this is corrected in patch 6, but in the meantime this can cause bisect issues. And besides, it's simply a bug :-) After changing this cast you can add my: Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> 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; >
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(-)