Em Tue, 17 Apr 2018 13:04:31 +0200 Hans Verkuil <hansv...@cisco.com> escreveu:
> On 04/17/18 12:53, Mauro Carvalho Chehab wrote: > > Em Tue, 17 Apr 2018 12:33:11 +0200 > > Hans Verkuil <hansv...@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 <mche...@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