[PATCH 0/2]

[PATCH 0/2]

[Laurent Pinchart]

Hello,

This small patch series simplifies colorspace handling for drivers by
sanitizing values in the V4L2 core.

Patch 1/2 improves the colorspace validity checks in existing helper
functions, to make them more future-proof. It's not a hard dependency
for the next patch, and could be dropped if desired.

Patch 2/2 then extends the v4l_sanitize_format() function to also
sanitize colorspace fields.

Laurent Pinchart (2):
  media: v4l2: Make colorspace validity checks more future-proof
  media: v4l2: Sanitize colorspace values in the framework

 drivers/media/v4l2-core/v4l2-ioctl.c | 65 +++++++++++++++++++++++-----
 include/media/v4l2-common.h          | 10 ++---
 include/uapi/linux/videodev2.h       | 29 +++++++++++++
 3 files changed, 89 insertions(+), 15 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] media: v4l2: Make colorspace validity checks more future-proof

[Laurent Pinchart]

The helper functions that test validity of colorspace-related fields
use the last value of the corresponding enums. This isn't very
future-proof, as there's a high chance someone adding a new value may
forget to update the helpers. Add new "LAST" entries to the enumerations
to improve this, and keep them private to the kernel.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/media/v4l2-common.h    | 10 +++++-----
 include/uapi/linux/videodev2.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 3eb202259e8c..b686124e2ccf 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -563,19 +563,19 @@ static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
 static inline bool v4l2_is_colorspace_valid(__u32 colorspace)
 {
 	return colorspace > V4L2_COLORSPACE_DEFAULT &&
-	       colorspace <= V4L2_COLORSPACE_DCI_P3;
+	       colorspace <= V4L2_COLORSPACE_LAST;
 }
 
 static inline bool v4l2_is_xfer_func_valid(__u32 xfer_func)
 {
 	return xfer_func > V4L2_XFER_FUNC_DEFAULT &&
-	       xfer_func <= V4L2_XFER_FUNC_SMPTE2084;
+	       xfer_func <= V4L2_XFER_FUNC_LAST;
 }
 
 static inline bool v4l2_is_ycbcr_enc_valid(__u8 ycbcr_enc)
 {
 	return ycbcr_enc > V4L2_YCBCR_ENC_DEFAULT &&
-	       ycbcr_enc <= V4L2_YCBCR_ENC_SMPTE240M;
+	       ycbcr_enc <= V4L2_YCBCR_ENC_LAST;
 }
 
 static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
@@ -585,8 +585,8 @@ static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
 
 static inline bool v4l2_is_quant_valid(__u8 quantization)
 {
-	return quantization == V4L2_QUANTIZATION_FULL_RANGE ||
-	       quantization == V4L2_QUANTIZATION_LIM_RANGE;
+	return quantization > V4L2_QUANTIZATION_DEFAULT &&
+	       quantization <= V4L2_QUANTIZATION_LAST;
 }
 
 #endif /* V4L2_COMMON_H_ */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 16dcd9dd1a50..099da1576db6 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -245,6 +245,14 @@ enum v4l2_colorspace {
 
 	/* DCI-P3 colorspace, used by cinema projectors */
 	V4L2_COLORSPACE_DCI_P3        = 12,
+
+#ifdef __KERNEL__
+	/*
+	 * Largest supported colorspace value, used by the framework to check
+	 * for invalid values.
+	 */
+	V4L2_COLORSPACE_LAST          = 12,
+#endif
 };
 
 /*
@@ -283,6 +291,13 @@ enum v4l2_xfer_func {
 	V4L2_XFER_FUNC_NONE        = 5,
 	V4L2_XFER_FUNC_DCI_P3      = 6,
 	V4L2_XFER_FUNC_SMPTE2084   = 7,
+#ifdef __KERNEL__
+	/*
+	 * Largest supported transfer function value, used by the framework to
+	 * check for invalid values.
+	 */
+	V4L2_XFER_FUNC_LAST        = 7,
+#endif
 };
 
 /*
@@ -343,6 +358,13 @@ enum v4l2_ycbcr_encoding {
 
 	/* SMPTE 240M -- Obsolete HDTV */
 	V4L2_YCBCR_ENC_SMPTE240M      = 8,
+#ifdef __KERNEL__
+	/*
+	 * Largest supported encoding value, used by the framework to check for
+	 * invalid values.
+	 */
+	V4L2_YCBCR_ENC_LAST           = 8,
+#endif
 };
 
 /*
@@ -378,6 +400,13 @@ enum v4l2_quantization {
 	V4L2_QUANTIZATION_DEFAULT     = 0,
 	V4L2_QUANTIZATION_FULL_RANGE  = 1,
 	V4L2_QUANTIZATION_LIM_RANGE   = 2,
+#ifdef __KERNEL__
+	/*
+	 * Largest supported quantization value, used by the framework to check
+	 * for invalid values.
+	 */
+	V4L2_QUANTIZATION_LAST        = 2,
+#endif
 };
 
 /*
-- 
Regards,

Laurent Pinchart

[PATCH 2/2] media: v4l2: Sanitize colorspace values in the framework

[Laurent Pinchart]

Extend the format sanitization code in the framework to handle invalid
values for the colorspace-related fields.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 65 +++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 4fe3f21f8bb4..01cfd3442ba5 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1006,6 +1006,31 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 	return -EINVAL;
 }
 
+static void v4l_sanitize_colorspace(u32 pixelformat, u32 *colorspace,
+				    u32 *encoding, u32 *quantization,
+				    u32 *xfer_func)
+{
+	bool is_hsv = pixelformat == V4L2_PIX_FMT_HSV24 ||
+		      pixelformat == V4L2_PIX_FMT_HSV32;
+
+	if (!v4l2_is_colorspace_valid(*colorspace)) {
+		*colorspace = V4L2_COLORSPACE_DEFAULT;
+		*encoding = V4L2_YCBCR_ENC_DEFAULT;
+		*quantization = V4L2_QUANTIZATION_DEFAULT;
+		*xfer_func = V4L2_XFER_FUNC_DEFAULT;
+	}
+
+	if ((!is_hsv && !v4l2_is_ycbcr_enc_valid(*encoding)) ||
+	    (is_hsv && !v4l2_is_hsv_enc_valid(*encoding)))
+		*encoding = V4L2_YCBCR_ENC_DEFAULT;
+
+	if (!v4l2_is_quant_valid(*quantization))
+		*quantization = V4L2_QUANTIZATION_DEFAULT;
+
+	if (!v4l2_is_xfer_func_valid(*xfer_func))
+		*xfer_func = V4L2_XFER_FUNC_DEFAULT;
+}
+
 static void v4l_sanitize_format(struct v4l2_format *fmt)
 {
 	unsigned int offset;
@@ -1025,20 +1050,40 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
 	 * field to the magic value when the extended pixel format structure
 	 * isn't used by applications.
 	 */
+	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
+	    fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+		if (fmt->fmt.pix.priv != V4L2_PIX_FMT_PRIV_MAGIC) {
+			fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
 
-	if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
-	    fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
-		return;
+			offset = offsetof(struct v4l2_pix_format, priv)
+			       + sizeof(fmt->fmt.pix.priv);
+			memset(((void *)&fmt->fmt.pix) + offset, 0,
+			       sizeof(fmt->fmt.pix) - offset);
+		}
+	}
 
-	if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC)
-		return;
+	/* Replace invalid colorspace values with defaults. */
+	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
+	    fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+		v4l_sanitize_colorspace(fmt->fmt.pix.pixelformat,
+					&fmt->fmt.pix.colorspace,
+					&fmt->fmt.pix.ycbcr_enc,
+					&fmt->fmt.pix.quantization,
+					&fmt->fmt.pix.xfer_func);
+	} else if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ||
+		   fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+		u32 ycbcr_enc = fmt->fmt.pix_mp.ycbcr_enc;
+		u32 quantization = fmt->fmt.pix_mp.quantization;
+		u32 xfer_func = fmt->fmt.pix_mp.xfer_func;
 
-	fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
+		v4l_sanitize_colorspace(fmt->fmt.pix_mp.pixelformat,
+					&fmt->fmt.pix_mp.colorspace, &ycbcr_enc,
+					&quantization, &xfer_func);
 
-	offset = offsetof(struct v4l2_pix_format, priv)
-	       + sizeof(fmt->fmt.pix.priv);
-	memset(((void *)&fmt->fmt.pix) + offset, 0,
-	       sizeof(fmt->fmt.pix) - offset);
+		fmt->fmt.pix_mp.ycbcr_enc = ycbcr_enc;
+		fmt->fmt.pix_mp.quantization = quantization;
+		fmt->fmt.pix_mp.xfer_func = xfer_func;
+	}
 }
 
 static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: v4l2: Make colorspace validity checks more future-proof

[Sakari Ailus]

Hi Laurent,

On Thu, Mar 17, 2022 at 04:36:59PM +0200, Laurent Pinchart wrote:
> The helper functions that test validity of colorspace-related fields
> use the last value of the corresponding enums. This isn't very
> future-proof, as there's a high chance someone adding a new value may
> forget to update the helpers. Add new "LAST" entries to the enumerations
> to improve this, and keep them private to the kernel.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/media/v4l2-common.h    | 10 +++++-----
>  include/uapi/linux/videodev2.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 3eb202259e8c..b686124e2ccf 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -563,19 +563,19 @@ static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
>  static inline bool v4l2_is_colorspace_valid(__u32 colorspace)
>  {
>  	return colorspace > V4L2_COLORSPACE_DEFAULT &&
> -	       colorspace <= V4L2_COLORSPACE_DCI_P3;
> +	       colorspace <= V4L2_COLORSPACE_LAST;
>  }
>  
>  static inline bool v4l2_is_xfer_func_valid(__u32 xfer_func)
>  {
>  	return xfer_func > V4L2_XFER_FUNC_DEFAULT &&
> -	       xfer_func <= V4L2_XFER_FUNC_SMPTE2084;
> +	       xfer_func <= V4L2_XFER_FUNC_LAST;
>  }
>  
>  static inline bool v4l2_is_ycbcr_enc_valid(__u8 ycbcr_enc)
>  {
>  	return ycbcr_enc > V4L2_YCBCR_ENC_DEFAULT &&
> -	       ycbcr_enc <= V4L2_YCBCR_ENC_SMPTE240M;
> +	       ycbcr_enc <= V4L2_YCBCR_ENC_LAST;
>  }
>  
>  static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
> @@ -585,8 +585,8 @@ static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
>  
>  static inline bool v4l2_is_quant_valid(__u8 quantization)
>  {
> -	return quantization == V4L2_QUANTIZATION_FULL_RANGE ||
> -	       quantization == V4L2_QUANTIZATION_LIM_RANGE;
> +	return quantization > V4L2_QUANTIZATION_DEFAULT &&
> +	       quantization <= V4L2_QUANTIZATION_LAST;
>  }
>  
>  #endif /* V4L2_COMMON_H_ */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 16dcd9dd1a50..099da1576db6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -245,6 +245,14 @@ enum v4l2_colorspace {
>  
>  	/* DCI-P3 colorspace, used by cinema projectors */
>  	V4L2_COLORSPACE_DCI_P3        = 12,
> +
> +#ifdef __KERNEL__
> +	/*
> +	 * Largest supported colorspace value, used by the framework to check
> +	 * for invalid values.
> +	 */
> +	V4L2_COLORSPACE_LAST          = 12,

I might just add the enum there, it is more obvious it needs updating if
it's right next to the previous one. Or rely on the compiler assigning the
value, and update the code. Up to you.

For both:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +#endif
>  };
>  
>  /*
> @@ -283,6 +291,13 @@ enum v4l2_xfer_func {
>  	V4L2_XFER_FUNC_NONE        = 5,
>  	V4L2_XFER_FUNC_DCI_P3      = 6,
>  	V4L2_XFER_FUNC_SMPTE2084   = 7,
> +#ifdef __KERNEL__
> +	/*
> +	 * Largest supported transfer function value, used by the framework to
> +	 * check for invalid values.
> +	 */
> +	V4L2_XFER_FUNC_LAST        = 7,
> +#endif
>  };
>  
>  /*
> @@ -343,6 +358,13 @@ enum v4l2_ycbcr_encoding {
>  
>  	/* SMPTE 240M -- Obsolete HDTV */
>  	V4L2_YCBCR_ENC_SMPTE240M      = 8,
> +#ifdef __KERNEL__
> +	/*
> +	 * Largest supported encoding value, used by the framework to check for
> +	 * invalid values.
> +	 */
> +	V4L2_YCBCR_ENC_LAST           = 8,
> +#endif
>  };
>  
>  /*
> @@ -378,6 +400,13 @@ enum v4l2_quantization {
>  	V4L2_QUANTIZATION_DEFAULT     = 0,
>  	V4L2_QUANTIZATION_FULL_RANGE  = 1,
>  	V4L2_QUANTIZATION_LIM_RANGE   = 2,
> +#ifdef __KERNEL__
> +	/*
> +	 * Largest supported quantization value, used by the framework to check
> +	 * for invalid values.
> +	 */
> +	V4L2_QUANTIZATION_LAST        = 2,
> +#endif
>  };
>  
>  /*

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/2] media: v4l2: Make colorspace validity checks more future-proof

[Laurent Pinchart]

Hi Sakari,

On Thu, Mar 17, 2022 at 06:06:21PM +0200, Sakari Ailus wrote:
> On Thu, Mar 17, 2022 at 04:36:59PM +0200, Laurent Pinchart wrote:
> > The helper functions that test validity of colorspace-related fields
> > use the last value of the corresponding enums. This isn't very
> > future-proof, as there's a high chance someone adding a new value may
> > forget to update the helpers. Add new "LAST" entries to the enumerations
> > to improve this, and keep them private to the kernel.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/media/v4l2-common.h    | 10 +++++-----
> >  include/uapi/linux/videodev2.h | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index 3eb202259e8c..b686124e2ccf 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -563,19 +563,19 @@ static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
> >  static inline bool v4l2_is_colorspace_valid(__u32 colorspace)
> >  {
> >  	return colorspace > V4L2_COLORSPACE_DEFAULT &&
> > -	       colorspace <= V4L2_COLORSPACE_DCI_P3;
> > +	       colorspace <= V4L2_COLORSPACE_LAST;
> >  }
> >  
> >  static inline bool v4l2_is_xfer_func_valid(__u32 xfer_func)
> >  {
> >  	return xfer_func > V4L2_XFER_FUNC_DEFAULT &&
> > -	       xfer_func <= V4L2_XFER_FUNC_SMPTE2084;
> > +	       xfer_func <= V4L2_XFER_FUNC_LAST;
> >  }
> >  
> >  static inline bool v4l2_is_ycbcr_enc_valid(__u8 ycbcr_enc)
> >  {
> >  	return ycbcr_enc > V4L2_YCBCR_ENC_DEFAULT &&
> > -	       ycbcr_enc <= V4L2_YCBCR_ENC_SMPTE240M;
> > +	       ycbcr_enc <= V4L2_YCBCR_ENC_LAST;
> >  }
> >  
> >  static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
> > @@ -585,8 +585,8 @@ static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
> >  
> >  static inline bool v4l2_is_quant_valid(__u8 quantization)
> >  {
> > -	return quantization == V4L2_QUANTIZATION_FULL_RANGE ||
> > -	       quantization == V4L2_QUANTIZATION_LIM_RANGE;
> > +	return quantization > V4L2_QUANTIZATION_DEFAULT &&
> > +	       quantization <= V4L2_QUANTIZATION_LAST;
> >  }
> >  
> >  #endif /* V4L2_COMMON_H_ */
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 16dcd9dd1a50..099da1576db6 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -245,6 +245,14 @@ enum v4l2_colorspace {
> >  
> >  	/* DCI-P3 colorspace, used by cinema projectors */
> >  	V4L2_COLORSPACE_DCI_P3        = 12,
> > +
> > +#ifdef __KERNEL__
> > +	/*
> > +	 * Largest supported colorspace value, used by the framework to check
> > +	 * for invalid values.
> > +	 */
> > +	V4L2_COLORSPACE_LAST          = 12,
> 
> I might just add the enum there, it is more obvious it needs updating if
> it's right next to the previous one.

I'm not sure to understand what you mean by "add the enum there".

> Or rely on the compiler assigning the
> value, and update the code. Up to you.

Good idea. Hans, what do you think ?

> For both:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> > +#endif
> >  };
> >  
> >  /*
> > @@ -283,6 +291,13 @@ enum v4l2_xfer_func {
> >  	V4L2_XFER_FUNC_NONE        = 5,
> >  	V4L2_XFER_FUNC_DCI_P3      = 6,
> >  	V4L2_XFER_FUNC_SMPTE2084   = 7,
> > +#ifdef __KERNEL__
> > +	/*
> > +	 * Largest supported transfer function value, used by the framework to
> > +	 * check for invalid values.
> > +	 */
> > +	V4L2_XFER_FUNC_LAST        = 7,
> > +#endif
> >  };
> >  
> >  /*
> > @@ -343,6 +358,13 @@ enum v4l2_ycbcr_encoding {
> >  
> >  	/* SMPTE 240M -- Obsolete HDTV */
> >  	V4L2_YCBCR_ENC_SMPTE240M      = 8,
> > +#ifdef __KERNEL__
> > +	/*
> > +	 * Largest supported encoding value, used by the framework to check for
> > +	 * invalid values.
> > +	 */
> > +	V4L2_YCBCR_ENC_LAST           = 8,
> > +#endif
> >  };
> >  
> >  /*
> > @@ -378,6 +400,13 @@ enum v4l2_quantization {
> >  	V4L2_QUANTIZATION_DEFAULT     = 0,
> >  	V4L2_QUANTIZATION_FULL_RANGE  = 1,
> >  	V4L2_QUANTIZATION_LIM_RANGE   = 2,
> > +#ifdef __KERNEL__
> > +	/*
> > +	 * Largest supported quantization value, used by the framework to check
> > +	 * for invalid values.
> > +	 */
> > +	V4L2_QUANTIZATION_LAST        = 2,
> > +#endif
> >  };
> >  
> >  /*

-- 
Regards,

Laurent Pinchart