Remaining COMPILE_TEST and smatch cleanups

PATCH Superseded Mauro Carvalho Chehab 4 participants 21 messages Apr 17, 2018 – Apr 23, 2018 1 message not in the archive Media (V4L/DVB)

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2018-04-17 10:20 UTC #0 lore raw

cover Remaining COMPILE_TEST and smatch cleanups

There were several interactions at the COMPILE_TEST and smatch patch series. While I applied most of them, there are 5 patches that I kept out of it. The omap3 patch that were in my tree was the old one. So, I'm re-posting it.

The ioctl32 patches are the latest version. Let's repost it to get some acks, as this patch touches at V4L2 core, so a careful review is always a good idea.

Arnd Bergmann (1):
media: omap3isp: allow it to build with COMPILE_TEST

Laurent Pinchart (1):
media: omap3isp: Enable driver compilation on ARM with COMPILE_TEST

Mauro Carvalho Chehab (3):
omap: omap-iommu.h: allow building drivers with COMPILE_TEST
media: v4l2-compat-ioctl32: fix several __user annotations
media: v4l2-compat-ioctl32: better name userspace pointers

 drivers/media/platform/Kconfig                |   7 +-
 drivers/media/platform/omap3isp/isp.c         |   8 +
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 623 +++++++++++++-------------
 include/linux/omap-iommu.h                    |   5 +
 4 files changed, 338 insertions(+), 305 deletions(-)

Signature · 1 line

2.14.3

Mauro Carvalho Chehab

author

2018-04-17 10:20 UTC #1 lore raw

PATCH 1/5 omap: omap-iommu.h: allow building drivers with COMPILE_TEST

Drivers that depend on omap-iommu.h (currently, just omap3isp) need a stub implementation in order to be built with COMPILE_TEST.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 include/linux/omap-iommu.h | 5 +++++
 1 file changed, 5 insertions(+)

include/linux/omap-iommu.h +5 -0

index c1aede46718b..ce1b7c6283ee 100644
@@ -13,7 +13,12 @@
 #ifndef _OMAP_IOMMU_H_
 #define _OMAP_IOMMU_H_
 
+#ifdef CONFIG_OMAP_IOMMU
 extern void omap_iommu_save_ctx(struct device *dev);
 extern void omap_iommu_restore_ctx(struct device *dev);
+#else
+static inline void omap_iommu_save_ctx(struct device *dev) {}
+static inline void omap_iommu_restore_ctx(struct device *dev) {}
+#endif
 
 #endif

Signature · 1 line

2.14.3

Mauro Carvalho Chehab

author

2018-04-17 10:20 UTC #2 lore raw

PATCH 2/5 media: omap3isp: Enable driver compilation on ARM with COMPILE_TEST

In reply to Mauro Carvalho Chehab

From: Laurent Pinchart <[email protected]>

The omap3isp driver can't be compiled on non-ARM platforms but has no compile-time dependency on OMAP. It however requires common clock framework support, which isn't provided by all ARM platforms.

Drop the OMAP dependency when COMPILE_TEST is set and add ARM and COMMON_CLK dependencies.

Signed-off-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/media/platform/Kconfig +4 -1

index 91b0c7324afb..1ee915b794c0 100644
@@ -62,7 +62,10 @@ config VIDEO_MUX
 
 config VIDEO_OMAP3
 	tristate "OMAP 3 Camera support"
-	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
+	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+	depends on ARCH_OMAP3 || COMPILE_TEST
+	depends on ARM
+	depends on COMMON_CLK
 	depends on HAS_DMA && OF
 	depends on OMAP_IOMMU
 	select ARM_DMA_USE_IOMMU

Signature · 1 line

2.14.3

Mauro Carvalho Chehab

author

2018-04-17 10:20 UTC #3 lore raw

PATCH 3/5 media: omap3isp: allow it to build with COMPILE_TEST

In reply to Mauro Carvalho Chehab

From: Arnd Bergmann <[email protected]>

There aren't much things required for it to build with COMPILE_TEST. It just needs to not compile the code that depends on arm-specific iommu implementation.

Signed-off-by Arnd Bergmann <[email protected]>

Co-developed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/Kconfig        | 6 ++----
 drivers/media/platform/omap3isp/isp.c | 8 ++++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

drivers/media/platform/Kconfig +2 -4

index 1ee915b794c0..2757b621091c 100644
@@ -63,12 +63,10 @@ config VIDEO_MUX
 config VIDEO_OMAP3
 	tristate "OMAP 3 Camera support"
 	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
-	depends on ARCH_OMAP3 || COMPILE_TEST
-	depends on ARM
+	depends on ((ARCH_OMAP3 && OMAP_IOMMU) || COMPILE_TEST)
 	depends on COMMON_CLK
 	depends on HAS_DMA && OF
-	depends on OMAP_IOMMU
-	select ARM_DMA_USE_IOMMU
+	select ARM_DMA_USE_IOMMU if OMAP_IOMMU
 	select VIDEOBUF2_DMA_CONTIG
 	select MFD_SYSCON
 	select V4L2_FWNODE

drivers/media/platform/omap3isp/isp.c +8 -0

index 16c50099cccd..b8c8761a76b6 100644
@@ -61,7 +61,9 @@
 #include <linux/sched.h>
 #include <linux/vmalloc.h>
 
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
 #include <asm/dma-iommu.h>
+#endif
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-fwnode.h>

@@ -1938,12 +1940,15 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 static void isp_detach_iommu(struct isp_device *isp)
 {
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
 	arm_iommu_release_mapping(isp->mapping);
 	isp->mapping = NULL;
+#endif
 }
 
 static int isp_attach_iommu(struct isp_device *isp)
 {
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
 	struct dma_iommu_mapping *mapping;
 	int ret;

@@ -1972,6 +1977,9 @@ static int isp_attach_iommu(struct isp_device *isp)
 error:
 	isp_detach_iommu(isp);
 	return ret;
+#else
+	return -ENODEV;
+#endif
 }
 
 /*

Signature · 1 line

2.14.3

Mauro Carvalho Chehab

author

2018-04-17 10:20 UTC #4 lore raw

PATCH 4/5 media: v4l2-compat-ioctl32: fix several __user annotations

In reply to Mauro Carvalho Chehab

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 <[email protected]>

---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +35 ~~-16~~

index d03a44d89649..c951ac3faf46 100644
@@ -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;

Signature · 1 line

2.14.3

Hans Verkuil

2018-04-17 10:33 UTC #5 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

Quoted 118 lines · Mauro Carvalho Chehab

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 <[email protected]>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +24 -9
index d03a44d89649..c951ac3faf46 100644
@@ -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) ||

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 901

Quoted 5 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

@@ -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.

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 904

Quoted 5 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -2

 		    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.

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 906

Quoted 2 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -0
+		    copy_in_user(&ucontrols->reserved2,
+				 (unsigned int __user *)&kcontrols->reserved2,

This can be a void __user *.

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 919

Quoted 10 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1

 				 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 *

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int get_v4l2_edid32(struct v4l2_edid __user *kp, · line 974

Quoted 10 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1

 			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.

Quoted 8 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

 	    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;

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_edid32(struct v4l2_edid __user *kp, · line 995

Quoted 8 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

@@ -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

Mauro Carvalho Chehab

author

2018-04-17 10:54 UTC #6 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

Em Tue, 17 Apr 2018 12:33:11 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 122 lines · Mauro Carvalho Chehab

Quoted 121 lines
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 <[email protected]>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +24 -9
index d03a44d89649..c951ac3faf46 100644
@@ -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;
 

Quoted 5 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 901
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1
-		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.

Quoted 9 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 904
Quoted 6 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -2
 		    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.

Quoted 4 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 906
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -0
+		    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)

Quoted 46 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 919

Quoted 11 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1

 				 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 *

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int get_v4l2_edid32(struct v4l2_edid __user *kp, · line 974

Quoted 10 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1

 			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.

Quoted 8 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

 	    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;

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_edid32(struct v4l2_edid __user *kp, · line 995

Quoted 8 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

@@ -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,

Quoted 1 line

Hans

Thanks,
Mauro

Hans Verkuil

2018-04-17 11:04 UTC #7 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

Quoted 136 lines · Mauro Carvalho Chehab

Em Tue, 17 Apr 2018 12:33:11 +0200
Hans Verkuil <[email protected]> escreveu:
Quoted 122 lines · Mauro Carvalho Chehab
Quoted 121 lines
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 <[email protected]>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +24 -9
index d03a44d89649..c951ac3faf46 100644
@@ -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;
 
Quoted 5 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 901
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1
-		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:

Quoted 8 lines

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.

Quoted 15 lines

Quoted 10 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 904
Quoted 7 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -2
 		    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.
Quoted 4 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 906
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -0
+		    copy_in_user(&ucontrols->reserved2,
+				 (unsigned int __user *)&kcontrols->reserved2,  
This can be a void __user *.

Quoted 7 lines

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).

Quoted 29 lines

(same applies to your similar comments below)
Quoted 26 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 919
Quoted 11 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1
 				 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 *
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int get_v4l2_edid32(struct v4l2_edid __user *kp, · line 974
Quoted 10 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1
 			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

Quoted 27 lines

Quoted 20 lines

Quoted 9 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

 	    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;

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_edid32(struct v4l2_edid __user *kp, · line 995

Quoted 8 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

@@ -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,

Quoted 1 line

Hans

Thanks,
Mauro

Mauro Carvalho Chehab

author

2018-04-17 11:31 UTC #8 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

Em Tue, 17 Apr 2018 13:04:31 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 144 lines · Mauro Carvalho Chehab

Quoted 143 lines
Em Tue, 17 Apr 2018 12:33:11 +0200
Hans Verkuil <[email protected]> escreveu:
Quoted 122 lines · Mauro Carvalho Chehab
Quoted 121 lines
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 <[email protected]>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +24 -9
index d03a44d89649..c951ac3faf46 100644
@@ -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;
 
Quoted 5 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 901
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1
-		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)

Quoted 3 lines

Quoted 1 line
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.

Quoted 21 lines

Quoted 21 lines
Quoted 11 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 904
Quoted 8 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -2
 		    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.
Quoted 4 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 906
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -0
+		    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.

Quoted 6 lines

Quoted 2 lines
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 *.

Quoted 31 lines

Quoted 30 lines
(same applies to your similar comments below)
Quoted 26 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 919
Quoted 11 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1
 				 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 *
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int get_v4l2_edid32(struct v4l2_edid __user *kp, · line 974
Quoted 10 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1
 			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.

Quoted 2 lines

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.

Quoted 33 lines

Regards,

Hans

Quoted 27 lines

Quoted 20 lines

Quoted 9 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

 	    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;

drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_edid32(struct v4l2_edid __user *kp, · line 995

Quoted 8 lines

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1

@@ -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,

Quoted 1 line

Hans

Thanks,
Mauro

Mauro Carvalho Chehab

author

2018-04-17 13:01 UTC #9 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

In reply to Hans Verkuil

Em Tue, 17 Apr 2018 13:04:31 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 205 lines · Mauro Carvalho Chehab

Quoted 136 lines
Em Tue, 17 Apr 2018 12:33:11 +0200
Hans Verkuil <[email protected]> escreveu:
Quoted 122 lines · Mauro Carvalho Chehab
Quoted 121 lines
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 <[email protected]>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +24 -9
index d03a44d89649..c951ac3faf46 100644
@@ -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;
 
Quoted 5 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 901
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +1 -1
-		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:
Quoted 8 lines
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.
Quoted 15 lines
Quoted 10 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 904
Quoted 7 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -2
 		    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.
Quoted 4 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 906
Quoted 2 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -0
+		    copy_in_user(&ucontrols->reserved2,
+				 (unsigned int __user *)&kcontrols->reserved2,    
This can be a void __user *.
Quoted 7 lines
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).
Quoted 29 lines
(same applies to your similar comments below)
Quoted 26 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int put_v4l2_ext_controls32(struct file *file, · line 919
Quoted 11 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1
 				 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 *
drivers/media/v4l2-core/v4l2-compat-ioctl32.c static int get_v4l2_edid32(struct v4l2_edid __user *kp, · line 974
Quoted 10 lines
drivers/media/v4l2-core/v4l2-compat-ioctl32.c +2 -1
 			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.

Quoted 6 lines

Be aware that the unsigned char * cast is actually a bug: it will clamp the u32 'blocks' value to a u8.
Regards,
Hans

What about this approach (code untested)?

Btw, I'm planning to do it as a separate patch... the p64/p32 patch makes a way harder to fold it back with patch 4/5:

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +22 ~~-11~~

index 8c05dd9660d3..4602a50211b9 100644
@@ -30,6 +30,16 @@
 	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
 })
 
+#define get_user_cast(__x, __ptr)					\
+({									\
+	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
+})
+
+#define put_user_force(__x, __ptr)					\
+({									\
+	put_user((typeof(*__x) __force *)(__x), __ptr);			\
+})
+
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long ret = -ENOIOCTLCMD;

@@ -543,8 +553,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
 			return -EFAULT;
 
 		uplane = aux_buf;
-		if (put_user((__force struct v4l2_plane *)uplane,
-			     &p64->m.planes))
+		if (put_user_force(uplane, &p64->m.planes))
 			return -EFAULT;
 
 		while (num_planes--) {

@@ -682,7 +691,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *p64,
 
 	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
 	    get_user(tmp, &p32->base) ||
-	    put_user((void __force *)compat_ptr(tmp), &p64->base) ||
+	    put_user_force(compat_ptr(tmp), &p64->base) ||
 	    assign_in_user(&p64->capability, &p32->capability) ||
 	    assign_in_user(&p64->flags, &p32->flags) ||
 	    copy_in_user(&p64->fmt, &p32->fmt, sizeof(p64->fmt)))

@@ -831,8 +840,7 @@ static int get_v4l2_ext_controls32(struct file *file,
 	if (aux_space < count * sizeof(*kcontrols))
 		return -EFAULT;
 	kcontrols = aux_buf;
-	if (put_user((__force struct v4l2_ext_control *)kcontrols,
-		     &p64->controls))
+	if (put_user_force(kcontrols, &p64->controls))
 		return -EFAULT;
 
 	for (n = 0; n < count; n++) {

@@ -859,6 +867,9 @@ static int get_v4l2_ext_controls32(struct file *file,
 	return 0;
 }
 
+
+
+
 static int put_v4l2_ext_controls32(struct file *file,
 				   struct v4l2_ext_controls __user *p64,
 				   struct v4l2_ext_controls32 __user *p32)

@@ -898,12 +909,12 @@ static int put_v4l2_ext_controls32(struct file *file,
 		unsigned int size = sizeof(*ucontrols);
 		u32 id;
 
-		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
+		if (get_user_cast(id, &kcontrols->id) ||
 		    put_user(id, &ucontrols->id) ||
 		    assign_in_user(&ucontrols->size,
-				   (unsigned int __user *)&kcontrols->size) ||
+				   (__u32 __user *)&kcontrols->size) ||
 		    copy_in_user(&ucontrols->reserved2,
-				 (unsigned int __user *)&kcontrols->reserved2,
+				 (void __user *)&kcontrols->reserved2,
 				 sizeof(ucontrols->reserved2)))
 			return -EFAULT;

@@ -916,7 +927,7 @@ static int put_v4l2_ext_controls32(struct file *file,
 			size -= sizeof(ucontrols->value64);
 
 		if (copy_in_user(ucontrols,
-			         (unsigned int __user *)kcontrols, size))
+			         (void __user *)kcontrols, size))
 			return -EFAULT;
 
 		ucontrols++;

@@ -971,9 +982,9 @@ static int get_v4l2_edid32(struct v4l2_edid __user *p64,
 	    assign_in_user(&p64->pad, &p32->pad) ||
 	    assign_in_user(&p64->start_block, &p32->start_block) ||
 	    assign_in_user(&p64->blocks,
-			   (unsigned char __user *)&p32->blocks) ||
+			   (__u32 __user *)&p32->blocks) ||
 	    get_user(tmp, &p32->edid) ||
-	    put_user((void __force *)compat_ptr(tmp), &p64->edid) ||
+	    put_user_force(compat_ptr(tmp), &p64->edid) ||
 	    copy_in_user(p64->reserved, p32->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
 	return 0;

Thanks,
Mauro

Mauro Carvalho Chehab

author

2018-04-17 13:10 UTC #10 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

Em Tue, 17 Apr 2018 10:01:31 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 10 lines

Quoted 8 lines
Quoted 1 line
Quoted 1 line
->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
What about this approach (code untested)?

Even better:

[PATCH] media: v4l2-compat-ioctl32: simplify casts

Making the cast right for get_user/put_user is not trivial, as it needs to ensure that the types are the correct ones.

Improve it by using macros.

PS.: Patch untested

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/v4l2-core/v4l2-compat-ioctl32.c +27 ~~-13~~

index 8c05dd9660d3..d2f0268427c2 100644
@@ -30,6 +30,24 @@
 	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
 })
 
+#define get_user_cast(__x, __ptr)					\
+({									\
+	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
+})
+
+#define put_user_force(__x, __ptr)					\
+({									\
+	put_user((typeof(*__x) __force *)(__x), __ptr);			\
+})
+
+#define assign_in_user_cast(to, from)					\
+({									\
+	typeof(*from) __assign_tmp;					\
+									\
+	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
+})
+
+
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long ret = -ENOIOCTLCMD;

@@ -543,8 +561,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
 			return -EFAULT;
 
 		uplane = aux_buf;
-		if (put_user((__force struct v4l2_plane *)uplane,
-			     &p64->m.planes))
+		if (put_user_force(uplane, &p64->m.planes))
 			return -EFAULT;
 
 		while (num_planes--) {

@@ -682,7 +699,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *p64,
 
 	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
 	    get_user(tmp, &p32->base) ||
-	    put_user((void __force *)compat_ptr(tmp), &p64->base) ||
+	    put_user_force(compat_ptr(tmp), &p64->base) ||
 	    assign_in_user(&p64->capability, &p32->capability) ||
 	    assign_in_user(&p64->flags, &p32->flags) ||
 	    copy_in_user(&p64->fmt, &p32->fmt, sizeof(p64->fmt)))

@@ -831,8 +848,7 @@ static int get_v4l2_ext_controls32(struct file *file,
 	if (aux_space < count * sizeof(*kcontrols))
 		return -EFAULT;
 	kcontrols = aux_buf;
-	if (put_user((__force struct v4l2_ext_control *)kcontrols,
-		     &p64->controls))
+	if (put_user_force(kcontrols, &p64->controls))
 		return -EFAULT;
 
 	for (n = 0; n < count; n++) {

@@ -898,12 +914,11 @@ static int put_v4l2_ext_controls32(struct file *file,
 		unsigned int size = sizeof(*ucontrols);
 		u32 id;
 
-		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
+		if (get_user_cast(id, &kcontrols->id) ||
 		    put_user(id, &ucontrols->id) ||
-		    assign_in_user(&ucontrols->size,
-				   (unsigned int __user *)&kcontrols->size) ||
+		    assign_in_user_cast(&ucontrols->size, &kcontrols->size) ||
 		    copy_in_user(&ucontrols->reserved2,
-				 (unsigned int __user *)&kcontrols->reserved2,
+				 (void __user *)&kcontrols->reserved2,
 				 sizeof(ucontrols->reserved2)))
 			return -EFAULT;

@@ -916,7 +931,7 @@ static int put_v4l2_ext_controls32(struct file *file,
 			size -= sizeof(ucontrols->value64);
 
 		if (copy_in_user(ucontrols,
-			         (unsigned int __user *)kcontrols, size))
+			         (void __user *)kcontrols, size))
 			return -EFAULT;
 
 		ucontrols++;

@@ -970,10 +985,9 @@ static int get_v4l2_edid32(struct v4l2_edid __user *p64,
 	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
 	    assign_in_user(&p64->pad, &p32->pad) ||
 	    assign_in_user(&p64->start_block, &p32->start_block) ||
-	    assign_in_user(&p64->blocks,
-			   (unsigned char __user *)&p32->blocks) ||
+	    assign_in_user_cast(&p64->blocks, &p32->blocks) ||
 	    get_user(tmp, &p32->edid) ||
-	    put_user((void __force *)compat_ptr(tmp), &p64->edid) ||
+	    put_user_force(compat_ptr(tmp), &p64->edid) ||
 	    copy_in_user(p64->reserved, p32->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
 	return 0;

Thanks,
Mauro

Mauro Carvalho Chehab

2018-04-17 13:58 UTC #11 lore raw

Re: [PATCH 4/5] media: v4l2-compat-ioctl32: fix several __user annotations

Em Tue, 17 Apr 2018 10:10:09 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 16 lines

Em Tue, 17 Apr 2018 10:01:31 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:
Quoted 10 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line
->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
What about this approach (code untested)?
Even better:

Quoted 8 lines

[PATCH] media: v4l2-compat-ioctl32: simplify casts
Making the cast right for get_user/put_user is not trivial, as it needs to ensure that the types are the correct ones.
Improve it by using macros.
PS.: Patch untested

Tested with:

	$ sudo modprobe vivid no_error_inj=1
	$ v4l2-compliance-32bits -a -s10 >32bits && v4l2-compliance-64bits -a -s10 > 64bits && diff 32bits 64bits

32 bits version results:

v4l2-compliance SHA : bc71e4a67c6fbc5940062843bc41e7c8679634ce

Compliance test for device /dev/video0:

Driver Info:

	Driver name      : vivid
	Card type        : vivid
	Bus info         : platform:vivid-000
	Driver version   : 4.17.0
	Capabilities     : 0x853f0df7

Video Capture
Video Output
Video Overlay
VBI Capture
VBI Output
Sliced VBI Capture
Sliced VBI Output
RDS Capture
RDS Output
SDR Capture
Tuner
HW Frequency Seek
Modulator
Audio
Radio
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x05230005
Video Capture
Video Overlay
Tuner
Audio
Read/Write
Streaming
Extended Pix Format

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second /dev/video0 open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK test VIDIOC_G/S_FREQUENCY: OK
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK
Inputs: 4 Audio Inputs: 2 Tuners: 1

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK
test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK
test VIDIOC_DV_TIMINGS_CAP: OK
test VIDIOC_G/S_EDID: OK

Control ioctls (Input 0):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 15 Private Controls: 39

Format ioctls (Input 0):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK

Control ioctls (Input 1):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 15 Private Controls: 39

Format ioctls (Input 1):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK
test Composing: OK
test Scaling: OK

Codec ioctls (Input 1):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 1):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK

Control ioctls (Input 2):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 15 Private Controls: 39

Format ioctls (Input 2):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK
test Composing: OK
test Scaling: OK

Codec ioctls (Input 2):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 2):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK

Control ioctls (Input 3):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
warn: v4l2-test-controls.cpp(825): V4L2_CID_DV_RX_POWER_PRESENT not found for input 3
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 15 Private Controls: 39

Format ioctls (Input 3):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK
test Composing: OK
test Scaling: OK

Codec ioctls (Input 3):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 3):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK

Test input 0:

Streaming ioctls:
test read/write: OK

Video Capture: Frame #000
Video Capture: Frame #001
Video Capture: Frame #002
Video Capture: Frame #003
Video Capture: Frame #004
Video Capture: Frame #005
Video Capture: Frame #006
Video Capture: Frame #007
Video Capture: Frame #008
Video Capture: Frame #009

Video Capture: Frame #000 (polling)
Video Capture: Frame #001 (polling)
Video Capture: Frame #002 (polling)
Video Capture: Frame #003 (polling)
Video Capture: Frame #004 (polling)
Video Capture: Frame #005 (polling)
Video Capture: Frame #006 (polling)
Video Capture: Frame #007 (polling)
Video Capture: Frame #008 (polling)
Video Capture: Frame #009 (polling)

test MMAP: OK

test USERPTR: OK
test DMABUF: Cannot test, specify --expbuf-device

Test input 1:

Streaming ioctls:
test read/write: OK
warn: v4l2-test-buffers.cpp(235): V4L2_BUF_FLAG_TIMECODE was used!

test MMAP: OK

test USERPTR: OK
test DMABUF: Cannot test, specify --expbuf-device

Test input 2:

Streaming ioctls:
test read/write: OK

test MMAP: OK

test USERPTR: OK
test DMABUF: Cannot test, specify --expbuf-device

Test input 3:

Streaming ioctls:
test read/write: OK

test MMAP: OK

test USERPTR: OK
test DMABUF: Cannot test, specify --expbuf-device

Total: 118, Succeeded: 118, Failed: 0, Warnings: 2

64 bits result:

v4l2-compliance SHA : bc71e4a67c6fbc5940062843bc41e7c8679634ce

Compliance test for device /dev/video0:

Driver Info:

	Driver name      : vivid
	Card type        : vivid
	Bus info         : platform:vivid-000
	Driver version   : 4.17.0
	Capabilities     : 0x853f0df7