[PATCH 00/10] drm/edid: Infoframe cleanups and fixes

[PATCH 00/10] drm/edid: Infoframe cleanups and fixes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This series tries to fix some issues with HDMI infoframes. In particular
we can currently send a bogus picture aspect ratio in the infoframe. I
included stuff to to make the infoframe unpakc more robust, evne though
we don't (yet) use it in drm. Additionally I included my earlier "empty"
HDMI infoframe support.

I have further work piled up on top which allows us to precompuet the
infoframes during the atomic check phase. But the series would have
become rather big, so I wanted to post these fixes and cleanups first.

Entire series (with the infoframe precompute) is available here:
git://github.com/vsyrjala/linux.git infoframe_precompute

Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Lin, Jia" <lin.a.jia@intel.com>
Cc: linux-media@vger.kernel.org
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Thierry Reding <thierry.reding@gmail.com>

Ville Syrjälä (10):
  video/hdmi: Allow "empty" HDMI infoframes
  drm/edid: Allow HDMI infoframe without VIC or S3D
  drm/modes: Introduce drm_mode_match()
  drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy
  drm/edid: Fix up edid_cea_modes[] formatting
  drm/edid: Fix cea mode aspect ratio handling
  drm/edid: Don't send bogus aspect ratios in AVI infoframes
  video/hdmi: Reject illegal picture aspect ratios
  video/hdmi: Constify 'buffer' to the unpack functions
  video/hdmi: Pass buffer size to infoframe unpack functions

 drivers/gpu/drm/bridge/sil-sii8620.c      |   3 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |   4 +-
 drivers/gpu/drm/drm_edid.c                | 159 +++++++++++++++++++-----------
 drivers/gpu/drm/drm_modes.c               | 134 +++++++++++++++++++------
 drivers/gpu/drm/exynos/exynos_hdmi.c      |   2 +-
 drivers/gpu/drm/i915/intel_hdmi.c         |  14 +--
 drivers/gpu/drm/mediatek/mtk_hdmi.c       |   3 +-
 drivers/gpu/drm/nouveau/nv50_display.c    |   3 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c      |   1 +
 drivers/gpu/drm/sti/sti_hdmi.c            |   4 +-
 drivers/gpu/drm/zte/zx_hdmi.c             |   1 +
 drivers/media/i2c/adv7511.c               |   2 +-
 drivers/media/i2c/adv7604.c               |   2 +-
 drivers/media/i2c/adv7842.c               |   2 +-
 drivers/media/i2c/tc358743.c              |   2 +-
 drivers/video/hdmi.c                      | 118 ++++++++++++++--------
 include/drm/drm_connector.h               |   5 +
 include/drm/drm_edid.h                    |   1 +
 include/drm/drm_modes.h                   |   9 ++
 include/linux/hdmi.h                      |   3 +-
 20 files changed, 326 insertions(+), 146 deletions(-)

-- 
2.13.6

[PATCH 01/10] video/hdmi: Allow "empty" HDMI infoframes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

HDMI 2.0 Appendix F suggest that we should keep sending the infoframe
when switching from 3D to 2D mode, even if the infoframe isn't strictly
necessary (ie. not needed to transmit the VIC or stereo information).
This is a workaround against some sinks that fail to realize that they
should switch from 3D to 2D mode when the source stop transmitting
the infoframe.

v2: Handle unpack() as well
    Pull the length calculation into a helper

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: linux-media@vger.kernel.org
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> #v1
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/hdmi.c | 51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 1cf907ecded4..111a0ab6280a 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -321,6 +321,17 @@ int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame)
 }
 EXPORT_SYMBOL(hdmi_vendor_infoframe_init);
 
+static int hdmi_vendor_infoframe_length(const struct hdmi_vendor_infoframe *frame)
+{
+	/* for side by side (half) we also need to provide 3D_Ext_Data */
+	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
+		return 6;
+	else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
+		return 5;
+	else
+		return 4;
+}
+
 /**
  * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary buffer
  * @frame: HDMI infoframe
@@ -341,19 +352,11 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
 	u8 *ptr = buffer;
 	size_t length;
 
-	/* empty info frame */
-	if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID)
-		return -EINVAL;
-
 	/* only one of those can be supplied */
 	if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
 		return -EINVAL;
 
-	/* for side by side (half) we also need to provide 3D_Ext_Data */
-	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
-		frame->length = 6;
-	else
-		frame->length = 5;
+	frame->length = hdmi_vendor_infoframe_length(frame);
 
 	length = HDMI_INFOFRAME_HEADER_SIZE + frame->length;
 
@@ -372,14 +375,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
 	ptr[5] = 0x0c;
 	ptr[6] = 0x00;
 
-	if (frame->vic) {
-		ptr[7] = 0x1 << 5;	/* video format */
-		ptr[8] = frame->vic;
-	} else {
+	if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) {
 		ptr[7] = 0x2 << 5;	/* video format */
 		ptr[8] = (frame->s3d_struct & 0xf) << 4;
 		if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
 			ptr[9] = (frame->s3d_ext_data & 0xf) << 4;
+	} else if (frame->vic) {
+		ptr[7] = 0x1 << 5;	/* video format */
+		ptr[8] = frame->vic;
+	} else {
+		ptr[7] = 0x0 << 5;	/* video format */
 	}
 
 	hdmi_infoframe_set_checksum(buffer, length);
@@ -1165,7 +1170,7 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
 
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
 	    ptr[1] != 1 ||
-	    (ptr[2] != 5 && ptr[2] != 6))
+	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
 		return -EINVAL;
 
 	length = ptr[2];
@@ -1193,16 +1198,22 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
 
 	hvf->length = length;
 
-	if (hdmi_video_format == 0x1) {
-		hvf->vic = ptr[4];
-	} else if (hdmi_video_format == 0x2) {
+	if (hdmi_video_format == 0x2) {
+		if (length != 5 && length != 6)
+			return -EINVAL;
 		hvf->s3d_struct = ptr[4] >> 4;
 		if (hvf->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) {
-			if (length == 6)
-				hvf->s3d_ext_data = ptr[5] >> 4;
-			else
+			if (length != 6)
 				return -EINVAL;
+			hvf->s3d_ext_data = ptr[5] >> 4;
 		}
+	} else if (hdmi_video_format == 0x1) {
+		if (length != 5)
+			return -EINVAL;
+		hvf->vic = ptr[4];
+	} else {
+		if (length != 4)
+			return -EINVAL;
 	}
 
 	return 0;
-- 
2.13.6

Re: [PATCH 01/10] video/hdmi: Allow "empty" HDMI infoframes

[Sharma, Shashank]

Regards

Shashank


On 11/13/2017 10:34 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> HDMI 2.0 Appendix F suggest that we should keep sending the infoframe
> when switching from 3D to 2D mode, even if the infoframe isn't strictly
> necessary (ie. not needed to transmit the VIC or stereo information).
> This is a workaround against some sinks that fail to realize that they
> should switch from 3D to 2D mode when the source stop transmitting
> the infoframe.
>
> v2: Handle unpack() as well
>      Pull the length calculation into a helper
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: linux-media@vger.kernel.org
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> #v1
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/video/hdmi.c | 51 +++++++++++++++++++++++++++++++--------------------
>   1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 1cf907ecded4..111a0ab6280a 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -321,6 +321,17 @@ int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame)
>   }
>   EXPORT_SYMBOL(hdmi_vendor_infoframe_init);
>   
> +static int hdmi_vendor_infoframe_length(const struct hdmi_vendor_infoframe *frame)
> +{
> +	/* for side by side (half) we also need to provide 3D_Ext_Data */
> +	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> +		return 6;
> +	else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
> +		return 5;
> +	else
> +		return 4;
> +}
> +
>   /**
>    * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary buffer
>    * @frame: HDMI infoframe
> @@ -341,19 +352,11 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>   	u8 *ptr = buffer;
>   	size_t length;
>   
> -	/* empty info frame */
> -	if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID)
> -		return -EINVAL;
> -
>   	/* only one of those can be supplied */
>   	if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
>   		return -EINVAL;
>   
> -	/* for side by side (half) we also need to provide 3D_Ext_Data */
> -	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> -		frame->length = 6;
> -	else
> -		frame->length = 5;
> +	frame->length = hdmi_vendor_infoframe_length(frame);
>   
>   	length = HDMI_INFOFRAME_HEADER_SIZE + frame->length;
>   
> @@ -372,14 +375,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>   	ptr[5] = 0x0c;
>   	ptr[6] = 0x00;
>   
> -	if (frame->vic) {
> -		ptr[7] = 0x1 << 5;	/* video format */
> -		ptr[8] = frame->vic;
> -	} else {
> +	if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) {
>   		ptr[7] = 0x2 << 5;	/* video format */
>   		ptr[8] = (frame->s3d_struct & 0xf) << 4;
>   		if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
>   			ptr[9] = (frame->s3d_ext_data & 0xf) << 4;
> +	} else if (frame->vic) {
Please correct me if I dint understand this properly, but for a HDMI 2.0 
sink + 3D transmission, wouldn't I be sending
HDMI 2.0 VIC = 94 as well as some valid s3d flag (like side by side) ?

- Shashank
> +		ptr[7] = 0x1 << 5;	/* video format */
> +		ptr[8] = frame->vic;
> +	} else {
> +		ptr[7] = 0x0 << 5;	/* video format */
>   	}
>   
>   	hdmi_infoframe_set_checksum(buffer, length);
> @@ -1165,7 +1170,7 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>   
>   	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
>   	    ptr[1] != 1 ||
> -	    (ptr[2] != 5 && ptr[2] != 6))
> +	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
>   		return -EINVAL;
>   
>   	length = ptr[2];
> @@ -1193,16 +1198,22 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>   
>   	hvf->length = length;
>   
> -	if (hdmi_video_format == 0x1) {
> -		hvf->vic = ptr[4];
> -	} else if (hdmi_video_format == 0x2) {
> +	if (hdmi_video_format == 0x2) {
> +		if (length != 5 && length != 6)
> +			return -EINVAL;
>   		hvf->s3d_struct = ptr[4] >> 4;
>   		if (hvf->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) {
> -			if (length == 6)
> -				hvf->s3d_ext_data = ptr[5] >> 4;
> -			else
> +			if (length != 6)
>   				return -EINVAL;
> +			hvf->s3d_ext_data = ptr[5] >> 4;
>   		}
> +	} else if (hdmi_video_format == 0x1) {
> +		if (length != 5)
> +			return -EINVAL;
> +		hvf->vic = ptr[4];
> +	} else {
> +		if (length != 4)
> +			return -EINVAL;
>   	}
>   
>   	return 0;

Re: [PATCH 01/10] video/hdmi: Allow "empty" HDMI infoframes

[Ville Syrjälä]

On Thu, Nov 16, 2017 at 08:06:18PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > HDMI 2.0 Appendix F suggest that we should keep sending the infoframe
> > when switching from 3D to 2D mode, even if the infoframe isn't strictly
> > necessary (ie. not needed to transmit the VIC or stereo information).
> > This is a workaround against some sinks that fail to realize that they
> > should switch from 3D to 2D mode when the source stop transmitting
> > the infoframe.
> >
> > v2: Handle unpack() as well
> >      Pull the length calculation into a helper
> >
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: linux-media@vger.kernel.org
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> #v1
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/video/hdmi.c | 51 +++++++++++++++++++++++++++++++--------------------
> >   1 file changed, 31 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> > index 1cf907ecded4..111a0ab6280a 100644
> > --- a/drivers/video/hdmi.c
> > +++ b/drivers/video/hdmi.c
> > @@ -321,6 +321,17 @@ int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame)
> >   }
> >   EXPORT_SYMBOL(hdmi_vendor_infoframe_init);
> >   
> > +static int hdmi_vendor_infoframe_length(const struct hdmi_vendor_infoframe *frame)
> > +{
> > +	/* for side by side (half) we also need to provide 3D_Ext_Data */
> > +	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> > +		return 6;
> > +	else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
> > +		return 5;
> > +	else
> > +		return 4;
> > +}
> > +
> >   /**
> >    * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary buffer
> >    * @frame: HDMI infoframe
> > @@ -341,19 +352,11 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
> >   	u8 *ptr = buffer;
> >   	size_t length;
> >   
> > -	/* empty info frame */
> > -	if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID)
> > -		return -EINVAL;
> > -
> >   	/* only one of those can be supplied */
> >   	if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
> >   		return -EINVAL;
> >   
> > -	/* for side by side (half) we also need to provide 3D_Ext_Data */
> > -	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> > -		frame->length = 6;
> > -	else
> > -		frame->length = 5;
> > +	frame->length = hdmi_vendor_infoframe_length(frame);
> >   
> >   	length = HDMI_INFOFRAME_HEADER_SIZE + frame->length;
> >   
> > @@ -372,14 +375,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
> >   	ptr[5] = 0x0c;
> >   	ptr[6] = 0x00;
> >   
> > -	if (frame->vic) {
> > -		ptr[7] = 0x1 << 5;	/* video format */
> > -		ptr[8] = frame->vic;
> > -	} else {
> > +	if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) {
> >   		ptr[7] = 0x2 << 5;	/* video format */
> >   		ptr[8] = (frame->s3d_struct & 0xf) << 4;
> >   		if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> >   			ptr[9] = (frame->s3d_ext_data & 0xf) << 4;
> > +	} else if (frame->vic) {
> Please correct me if I dint understand this properly, but for a HDMI 2.0 
> sink + 3D transmission, wouldn't I be sending
> HDMI 2.0 VIC = 94 as well as some valid s3d flag (like side by side) ?

That vic will be in the AVI infoframe. Here we're concerned about the
VIC in the HDMI vendor infoframe.

> 
> - Shashank
> > +		ptr[7] = 0x1 << 5;	/* video format */
> > +		ptr[8] = frame->vic;
> > +	} else {
> > +		ptr[7] = 0x0 << 5;	/* video format */
> >   	}
> >   
> >   	hdmi_infoframe_set_checksum(buffer, length);
> > @@ -1165,7 +1170,7 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> >   
> >   	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
> >   	    ptr[1] != 1 ||
> > -	    (ptr[2] != 5 && ptr[2] != 6))
> > +	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
> >   		return -EINVAL;
> >   
> >   	length = ptr[2];
> > @@ -1193,16 +1198,22 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> >   
> >   	hvf->length = length;
> >   
> > -	if (hdmi_video_format == 0x1) {
> > -		hvf->vic = ptr[4];
> > -	} else if (hdmi_video_format == 0x2) {
> > +	if (hdmi_video_format == 0x2) {
> > +		if (length != 5 && length != 6)
> > +			return -EINVAL;
> >   		hvf->s3d_struct = ptr[4] >> 4;
> >   		if (hvf->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) {
> > -			if (length == 6)
> > -				hvf->s3d_ext_data = ptr[5] >> 4;
> > -			else
> > +			if (length != 6)
> >   				return -EINVAL;
> > +			hvf->s3d_ext_data = ptr[5] >> 4;
> >   		}
> > +	} else if (hdmi_video_format == 0x1) {
> > +		if (length != 5)
> > +			return -EINVAL;
> > +		hvf->vic = ptr[4];
> > +	} else {
> > +		if (length != 4)
> > +			return -EINVAL;
> >   	}
> >   
> >   	return 0;

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 01/10] video/hdmi: Allow "empty" HDMI infoframes

[Sharma, Shashank]

Regards

Shashank


On 11/16/2017 9:46 PM, Ville Syrjälä wrote:
> On Thu, Nov 16, 2017 at 08:06:18PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> HDMI 2.0 Appendix F suggest that we should keep sending the infoframe
>>> when switching from 3D to 2D mode, even if the infoframe isn't strictly
>>> necessary (ie. not needed to transmit the VIC or stereo information).
>>> This is a workaround against some sinks that fail to realize that they
>>> should switch from 3D to 2D mode when the source stop transmitting
>>> the infoframe.
>>>
>>> v2: Handle unpack() as well
>>>       Pull the length calculation into a helper
>>>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>>> Cc: linux-media@vger.kernel.org
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> #v1
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/video/hdmi.c | 51 +++++++++++++++++++++++++++++++--------------------
>>>    1 file changed, 31 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
>>> index 1cf907ecded4..111a0ab6280a 100644
>>> --- a/drivers/video/hdmi.c
>>> +++ b/drivers/video/hdmi.c
>>> @@ -321,6 +321,17 @@ int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame)
>>>    }
>>>    EXPORT_SYMBOL(hdmi_vendor_infoframe_init);
>>>    
>>> +static int hdmi_vendor_infoframe_length(const struct hdmi_vendor_infoframe *frame)
>>> +{
>>> +	/* for side by side (half) we also need to provide 3D_Ext_Data */
>>> +	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
>>> +		return 6;
>>> +	else if (frame->vic != 0 || frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
>>> +		return 5;
>>> +	else
>>> +		return 4;
>>> +}
>>> +
>>>    /**
>>>     * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary buffer
>>>     * @frame: HDMI infoframe
>>> @@ -341,19 +352,11 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>>>    	u8 *ptr = buffer;
>>>    	size_t length;
>>>    
>>> -	/* empty info frame */
>>> -	if (frame->vic == 0 && frame->s3d_struct == HDMI_3D_STRUCTURE_INVALID)
>>> -		return -EINVAL;
>>> -
>>>    	/* only one of those can be supplied */
>>>    	if (frame->vic != 0 && frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID)
>>>    		return -EINVAL;
>>>    
>>> -	/* for side by side (half) we also need to provide 3D_Ext_Data */
>>> -	if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
>>> -		frame->length = 6;
>>> -	else
>>> -		frame->length = 5;
>>> +	frame->length = hdmi_vendor_infoframe_length(frame);
>>>    
>>>    	length = HDMI_INFOFRAME_HEADER_SIZE + frame->length;
>>>    
>>> @@ -372,14 +375,16 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>>>    	ptr[5] = 0x0c;
>>>    	ptr[6] = 0x00;
>>>    
>>> -	if (frame->vic) {
>>> -		ptr[7] = 0x1 << 5;	/* video format */
>>> -		ptr[8] = frame->vic;
>>> -	} else {
>>> +	if (frame->s3d_struct != HDMI_3D_STRUCTURE_INVALID) {
>>>    		ptr[7] = 0x2 << 5;	/* video format */
>>>    		ptr[8] = (frame->s3d_struct & 0xf) << 4;
>>>    		if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
>>>    			ptr[9] = (frame->s3d_ext_data & 0xf) << 4;
>>> +	} else if (frame->vic) {
>> Please correct me if I dint understand this properly, but for a HDMI 2.0
>> sink + 3D transmission, wouldn't I be sending
>> HDMI 2.0 VIC = 94 as well as some valid s3d flag (like side by side) ?
> That vic will be in the AVI infoframe. Here we're concerned about the
> VIC in the HDMI vendor infoframe.
Yep, I was thinking if there is any way we can cross check that there is 
a valid HDMI 2 vic
before we do anything here, but seems like a long shot.

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>> - Shashank
>>> +		ptr[7] = 0x1 << 5;	/* video format */
>>> +		ptr[8] = frame->vic;
>>> +	} else {
>>> +		ptr[7] = 0x0 << 5;	/* video format */
>>>    	}
>>>    
>>>    	hdmi_infoframe_set_checksum(buffer, length);
>>> @@ -1165,7 +1170,7 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>>>    
>>>    	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
>>>    	    ptr[1] != 1 ||
>>> -	    (ptr[2] != 5 && ptr[2] != 6))
>>> +	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
>>>    		return -EINVAL;
>>>    
>>>    	length = ptr[2];
>>> @@ -1193,16 +1198,22 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>>>    
>>>    	hvf->length = length;
>>>    
>>> -	if (hdmi_video_format == 0x1) {
>>> -		hvf->vic = ptr[4];
>>> -	} else if (hdmi_video_format == 0x2) {
>>> +	if (hdmi_video_format == 0x2) {
>>> +		if (length != 5 && length != 6)
>>> +			return -EINVAL;
>>>    		hvf->s3d_struct = ptr[4] >> 4;
>>>    		if (hvf->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF) {
>>> -			if (length == 6)
>>> -				hvf->s3d_ext_data = ptr[5] >> 4;
>>> -			else
>>> +			if (length != 6)
>>>    				return -EINVAL;
>>> +			hvf->s3d_ext_data = ptr[5] >> 4;
>>>    		}
>>> +	} else if (hdmi_video_format == 0x1) {
>>> +		if (length != 5)
>>> +			return -EINVAL;
>>> +		hvf->vic = ptr[4];
>>> +	} else {
>>> +		if (length != 4)
>>> +			return -EINVAL;
>>>    	}
>>>    
>>>    	return 0;

[PATCH 08/10] video/hdmi: Reject illegal picture aspect ratios

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

AVI infoframe can only carry none, 4:3, or 16:9 picture aspect
ratios. Return an error if the user asked for something different.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: "Lin, Jia" <lin.a.jia@intel.com>
Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/hdmi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 111a0ab6280a..38716eb50408 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -93,6 +93,9 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
 	if (size < length)
 		return -ENOSPC;
 
+	if (frame->picture_aspect > HDMI_PICTURE_ASPECT_16_9)
+		return -EINVAL;
+
 	memset(buffer, 0, size);
 
 	ptr[0] = frame->type;
-- 
2.13.6

Re: [PATCH 08/10] video/hdmi: Reject illegal picture aspect ratios

[Jose Abreu]

On 13-11-2017 17:04, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> AVI infoframe can only carry none, 4:3, or 16:9 picture aspect
> ratios. Return an error if the user asked for something different.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: "Lin, Jia" <lin.a.jia@intel.com>
> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Best Regards,
Jose Miguel Abreu

[PATCH 09/10] video/hdmi: Constify 'buffer' to the unpack functions

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The unpack functions just read from the passed in buffer,
so make it const.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/hdmi.c | 23 ++++++++++++-----------
 include/linux/hdmi.h |  3 ++-
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 38716eb50408..65b915ea4936 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -31,7 +31,7 @@
 
 #define hdmi_log(fmt, ...) dev_printk(level, dev, fmt, ##__VA_ARGS__)
 
-static u8 hdmi_infoframe_checksum(u8 *ptr, size_t size)
+static u8 hdmi_infoframe_checksum(const u8 *ptr, size_t size)
 {
 	u8 csum = 0;
 	size_t i;
@@ -1016,9 +1016,9 @@ EXPORT_SYMBOL(hdmi_infoframe_log);
  * Returns 0 on success or a negative error code on failure.
  */
 static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
-				     void *buffer)
+				     const void *buffer)
 {
-	u8 *ptr = buffer;
+	const u8 *ptr = buffer;
 	int ret;
 
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_AVI ||
@@ -1079,9 +1079,9 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
  * Returns 0 on success or a negative error code on failure.
  */
 static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
-				     void *buffer)
+				     const void *buffer)
 {
-	u8 *ptr = buffer;
+	const u8 *ptr = buffer;
 	int ret;
 
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_SPD ||
@@ -1117,9 +1117,9 @@ static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
  * Returns 0 on success or a negative error code on failure.
  */
 static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
-				       void *buffer)
+				       const void *buffer)
 {
-	u8 *ptr = buffer;
+	const u8 *ptr = buffer;
 	int ret;
 
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_AUDIO ||
@@ -1163,9 +1163,9 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
  */
 static int
 hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
-				 void *buffer)
+				 const void *buffer)
 {
-	u8 *ptr = buffer;
+	const u8 *ptr = buffer;
 	size_t length;
 	int ret;
 	u8 hdmi_video_format;
@@ -1234,10 +1234,11 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
  *
  * Returns 0 on success or a negative error code on failure.
  */
-int hdmi_infoframe_unpack(union hdmi_infoframe *frame, void *buffer)
+int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
+			  const void *buffer)
 {
 	int ret;
-	u8 *ptr = buffer;
+	const u8 *ptr = buffer;
 
 	switch (ptr[0]) {
 	case HDMI_INFOFRAME_TYPE_AVI:
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index d271ff23984f..d3816170c062 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -332,7 +332,8 @@ union hdmi_infoframe {
 
 ssize_t
 hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size);
-int hdmi_infoframe_unpack(union hdmi_infoframe *frame, void *buffer);
+int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
+			  const void *buffer);
 void hdmi_infoframe_log(const char *level, struct device *dev,
 			union hdmi_infoframe *frame);
 
-- 
2.13.6

[PATCH 10/10] video/hdmi: Pass buffer size to infoframe unpack functions

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To make sure the infoframe unpack functions don't end up examining
stack garbage or oopsing, let's pass in the size of the buffer.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/media/i2c/adv7511.c  |  2 +-
 drivers/media/i2c/adv7604.c  |  2 +-
 drivers/media/i2c/adv7842.c  |  2 +-
 drivers/media/i2c/tc358743.c |  2 +-
 drivers/video/hdmi.c         | 51 ++++++++++++++++++++++++++++++++------------
 include/linux/hdmi.h         |  2 +-
 6 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c
index 2817bafc67bf..dec09c18ea34 100644
--- a/drivers/media/i2c/adv7511.c
+++ b/drivers/media/i2c/adv7511.c
@@ -562,7 +562,7 @@ static void log_infoframe(struct v4l2_subdev *sd, const struct adv7511_cfg_read_
 	buffer[3] = 0;
 	buffer[3] = hdmi_infoframe_checksum(buffer, len + 4);
 
-	if (hdmi_infoframe_unpack(&frame, buffer) < 0) {
+	if (hdmi_infoframe_unpack(&frame, buffer, sizeof(buffer)) < 0) {
 		v4l2_err(sd, "%s: unpack of %s infoframe failed\n", __func__, cri->desc);
 		return;
 	}
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index f289b8aca1da..8500438af0d3 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2429,7 +2429,7 @@ static int adv76xx_read_infoframe(struct v4l2_subdev *sd, int index,
 		buffer[i + 3] = infoframe_read(sd,
 				       adv76xx_cri[index].payload_addr + i);
 
-	if (hdmi_infoframe_unpack(frame, buffer) < 0) {
+	if (hdmi_infoframe_unpack(frame, buffer, sizeof(buffer)) < 0) {
 		v4l2_err(sd, "%s: unpack of %s infoframe failed\n", __func__,
 			 adv76xx_cri[index].desc);
 		return -ENOENT;
diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 65f34e7e146f..fd5d5e84dcbf 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -2576,7 +2576,7 @@ static void log_infoframe(struct v4l2_subdev *sd, struct adv7842_cfg_read_infofr
 	for (i = 0; i < len; i++)
 		buffer[i + 3] = infoframe_read(sd, cri->payload_addr + i);
 
-	if (hdmi_infoframe_unpack(&frame, buffer) < 0) {
+	if (hdmi_infoframe_unpack(&frame, buffer, sizeof(buffer)) < 0) {
 		v4l2_err(sd, "%s: unpack of %s infoframe failed\n", __func__, cri->desc);
 		return;
 	}
diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c363ccab..f6a5ebffd9c6 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -453,7 +453,7 @@ static void print_avi_infoframe(struct v4l2_subdev *sd)
 
 	i2c_rd(sd, PK_AVI_0HEAD, buffer, HDMI_INFOFRAME_SIZE(AVI));
 
-	if (hdmi_infoframe_unpack(&frame, buffer) < 0) {
+	if (hdmi_infoframe_unpack(&frame, buffer, sizeof(buffer)) < 0) {
 		v4l2_err(sd, "%s: unpack of AVI infoframe failed\n", __func__);
 		return;
 	}
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 65b915ea4936..b5d491014b0b 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -1005,8 +1005,9 @@ EXPORT_SYMBOL(hdmi_infoframe_log);
 
 /**
  * hdmi_avi_infoframe_unpack() - unpack binary buffer to a HDMI AVI infoframe
- * @buffer: source buffer
  * @frame: HDMI AVI infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
  *
  * Unpacks the information contained in binary @buffer into a structured
  * @frame of the HDMI Auxiliary Video (AVI) information frame.
@@ -1016,11 +1017,14 @@ EXPORT_SYMBOL(hdmi_infoframe_log);
  * Returns 0 on success or a negative error code on failure.
  */
 static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
-				     const void *buffer)
+				     const void *buffer, size_t size)
 {
 	const u8 *ptr = buffer;
 	int ret;
 
+	if (size < HDMI_INFOFRAME_SIZE(AVI))
+		return -EINVAL;
+
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_AVI ||
 	    ptr[1] != 2 ||
 	    ptr[2] != HDMI_AVI_INFOFRAME_SIZE)
@@ -1068,8 +1072,9 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
 
 /**
  * hdmi_spd_infoframe_unpack() - unpack binary buffer to a HDMI SPD infoframe
- * @buffer: source buffer
  * @frame: HDMI SPD infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
  *
  * Unpacks the information contained in binary @buffer into a structured
  * @frame of the HDMI Source Product Description (SPD) information frame.
@@ -1079,11 +1084,14 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
  * Returns 0 on success or a negative error code on failure.
  */
 static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
-				     const void *buffer)
+				     const void *buffer, size_t size)
 {
 	const u8 *ptr = buffer;
 	int ret;
 
+	if (size < HDMI_INFOFRAME_SIZE(SPD))
+		return -EINVAL;
+
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_SPD ||
 	    ptr[1] != 1 ||
 	    ptr[2] != HDMI_SPD_INFOFRAME_SIZE) {
@@ -1106,8 +1114,9 @@ static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
 
 /**
  * hdmi_audio_infoframe_unpack() - unpack binary buffer to a HDMI AUDIO infoframe
- * @buffer: source buffer
  * @frame: HDMI Audio infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
  *
  * Unpacks the information contained in binary @buffer into a structured
  * @frame of the HDMI Audio information frame.
@@ -1117,11 +1126,14 @@ static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
  * Returns 0 on success or a negative error code on failure.
  */
 static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
-				       const void *buffer)
+				       const void *buffer, size_t size)
 {
 	const u8 *ptr = buffer;
 	int ret;
 
+	if (size < HDMI_INFOFRAME_SIZE(AUDIO))
+		return -EINVAL;
+
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_AUDIO ||
 	    ptr[1] != 1 ||
 	    ptr[2] != HDMI_AUDIO_INFOFRAME_SIZE) {
@@ -1151,8 +1163,9 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
 
 /**
  * hdmi_vendor_infoframe_unpack() - unpack binary buffer to a HDMI vendor infoframe
- * @buffer: source buffer
  * @frame: HDMI Vendor infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
  *
  * Unpacks the information contained in binary @buffer into a structured
  * @frame of the HDMI Vendor information frame.
@@ -1163,7 +1176,7 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
  */
 static int
 hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
-				 const void *buffer)
+				 const void *buffer, size_t size)
 {
 	const u8 *ptr = buffer;
 	size_t length;
@@ -1171,6 +1184,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
 	u8 hdmi_video_format;
 	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
 
+	if (size < HDMI_INFOFRAME_HEADER_SIZE)
+		return -EINVAL;
+
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
 	    ptr[1] != 1 ||
 	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
@@ -1178,6 +1194,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
 
 	length = ptr[2];
 
+	if (size < HDMI_INFOFRAME_HEADER_SIZE + length)
+		return -EINVAL;
+
 	if (hdmi_infoframe_checksum(buffer,
 				    HDMI_INFOFRAME_HEADER_SIZE + length) != 0)
 		return -EINVAL;
@@ -1224,8 +1243,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
 
 /**
  * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe
- * @buffer: source buffer
  * @frame: HDMI infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
  *
  * Unpacks the information contained in binary buffer @buffer into a structured
  * @frame of a HDMI infoframe.
@@ -1235,23 +1255,26 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
  * Returns 0 on success or a negative error code on failure.
  */
 int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
-			  const void *buffer)
+			  const void *buffer, size_t size)
 {
 	int ret;
 	const u8 *ptr = buffer;
 
+	if (size < HDMI_INFOFRAME_HEADER_SIZE)
+		return -EINVAL;
+
 	switch (ptr[0]) {
 	case HDMI_INFOFRAME_TYPE_AVI:
-		ret = hdmi_avi_infoframe_unpack(&frame->avi, buffer);
+		ret = hdmi_avi_infoframe_unpack(&frame->avi, buffer, size);
 		break;
 	case HDMI_INFOFRAME_TYPE_SPD:
-		ret = hdmi_spd_infoframe_unpack(&frame->spd, buffer);
+		ret = hdmi_spd_infoframe_unpack(&frame->spd, buffer, size);
 		break;
 	case HDMI_INFOFRAME_TYPE_AUDIO:
-		ret = hdmi_audio_infoframe_unpack(&frame->audio, buffer);
+		ret = hdmi_audio_infoframe_unpack(&frame->audio, buffer, size);
 		break;
 	case HDMI_INFOFRAME_TYPE_VENDOR:
-		ret = hdmi_vendor_any_infoframe_unpack(&frame->vendor, buffer);
+		ret = hdmi_vendor_any_infoframe_unpack(&frame->vendor, buffer, size);
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index d3816170c062..a577d4ae2570 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -333,7 +333,7 @@ union hdmi_infoframe {
 ssize_t
 hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size);
 int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
-			  const void *buffer);
+			  const void *buffer, size_t size);
 void hdmi_infoframe_log(const char *level, struct device *dev,
 			union hdmi_infoframe *frame);
 
-- 
2.13.6

Re: [PATCH 10/10] video/hdmi: Pass buffer size to infoframe unpack functions

[Hans Verkuil]

On 11/13/2017 06:04 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To make sure the infoframe unpack functions don't end up examining
> stack garbage or oopsing, let's pass in the size of the buffer.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/media/i2c/adv7511.c  |  2 +-
>  drivers/media/i2c/adv7604.c  |  2 +-
>  drivers/media/i2c/adv7842.c  |  2 +-
>  drivers/media/i2c/tc358743.c |  2 +-
>  drivers/video/hdmi.c         | 51 ++++++++++++++++++++++++++++++++------------
>  include/linux/hdmi.h         |  2 +-
>  6 files changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c
> index 2817bafc67bf..dec09c18ea34 100644
> --- a/drivers/media/i2c/adv7511.c
> +++ b/drivers/media/i2c/adv7511.c
> @@ -562,7 +562,7 @@ static void log_infoframe(struct v4l2_subdev *sd, const struct adv7511_cfg_read_
>  	buffer[3] = 0;
>  	buffer[3] = hdmi_infoframe_checksum(buffer, len + 4);
>  
> -	if (hdmi_infoframe_unpack(&frame, buffer) < 0) {
> +	if (hdmi_infoframe_unpack(&frame, buffer, sizeof(buffer)) < 0) {
>  		v4l2_err(sd, "%s: unpack of %s infoframe failed\n", __func__, cri->desc);
>  		return;
>  	}
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index f289b8aca1da..8500438af0d3 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -2429,7 +2429,7 @@ static int adv76xx_read_infoframe(struct v4l2_subdev *sd, int index,
>  		buffer[i + 3] = infoframe_read(sd,
>  				       adv76xx_cri[index].payload_addr + i);
>  
> -	if (hdmi_infoframe_unpack(frame, buffer) < 0) {
> +	if (hdmi_infoframe_unpack(frame, buffer, sizeof(buffer)) < 0) {
>  		v4l2_err(sd, "%s: unpack of %s infoframe failed\n", __func__,
>  			 adv76xx_cri[index].desc);
>  		return -ENOENT;
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index 65f34e7e146f..fd5d5e84dcbf 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -2576,7 +2576,7 @@ static void log_infoframe(struct v4l2_subdev *sd, struct adv7842_cfg_read_infofr
>  	for (i = 0; i < len; i++)
>  		buffer[i + 3] = infoframe_read(sd, cri->payload_addr + i);
>  
> -	if (hdmi_infoframe_unpack(&frame, buffer) < 0) {
> +	if (hdmi_infoframe_unpack(&frame, buffer, sizeof(buffer)) < 0) {
>  		v4l2_err(sd, "%s: unpack of %s infoframe failed\n", __func__, cri->desc);
>  		return;
>  	}
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index e6f5c363ccab..f6a5ebffd9c6 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -453,7 +453,7 @@ static void print_avi_infoframe(struct v4l2_subdev *sd)
>  
>  	i2c_rd(sd, PK_AVI_0HEAD, buffer, HDMI_INFOFRAME_SIZE(AVI));
>  
> -	if (hdmi_infoframe_unpack(&frame, buffer) < 0) {
> +	if (hdmi_infoframe_unpack(&frame, buffer, sizeof(buffer)) < 0) {
>  		v4l2_err(sd, "%s: unpack of AVI infoframe failed\n", __func__);
>  		return;
>  	}
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 65b915ea4936..b5d491014b0b 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -1005,8 +1005,9 @@ EXPORT_SYMBOL(hdmi_infoframe_log);
>  
>  /**
>   * hdmi_avi_infoframe_unpack() - unpack binary buffer to a HDMI AVI infoframe
> - * @buffer: source buffer
>   * @frame: HDMI AVI infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
>   *
>   * Unpacks the information contained in binary @buffer into a structured
>   * @frame of the HDMI Auxiliary Video (AVI) information frame.
> @@ -1016,11 +1017,14 @@ EXPORT_SYMBOL(hdmi_infoframe_log);
>   * Returns 0 on success or a negative error code on failure.
>   */
>  static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
> -				     const void *buffer)
> +				     const void *buffer, size_t size)
>  {
>  	const u8 *ptr = buffer;
>  	int ret;
>  
> +	if (size < HDMI_INFOFRAME_SIZE(AVI))
> +		return -EINVAL;
> +
>  	if (ptr[0] != HDMI_INFOFRAME_TYPE_AVI ||
>  	    ptr[1] != 2 ||
>  	    ptr[2] != HDMI_AVI_INFOFRAME_SIZE)
> @@ -1068,8 +1072,9 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
>  
>  /**
>   * hdmi_spd_infoframe_unpack() - unpack binary buffer to a HDMI SPD infoframe
> - * @buffer: source buffer
>   * @frame: HDMI SPD infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
>   *
>   * Unpacks the information contained in binary @buffer into a structured
>   * @frame of the HDMI Source Product Description (SPD) information frame.
> @@ -1079,11 +1084,14 @@ static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
>   * Returns 0 on success or a negative error code on failure.
>   */
>  static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
> -				     const void *buffer)
> +				     const void *buffer, size_t size)
>  {
>  	const u8 *ptr = buffer;
>  	int ret;
>  
> +	if (size < HDMI_INFOFRAME_SIZE(SPD))
> +		return -EINVAL;
> +
>  	if (ptr[0] != HDMI_INFOFRAME_TYPE_SPD ||
>  	    ptr[1] != 1 ||
>  	    ptr[2] != HDMI_SPD_INFOFRAME_SIZE) {
> @@ -1106,8 +1114,9 @@ static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
>  
>  /**
>   * hdmi_audio_infoframe_unpack() - unpack binary buffer to a HDMI AUDIO infoframe
> - * @buffer: source buffer
>   * @frame: HDMI Audio infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
>   *
>   * Unpacks the information contained in binary @buffer into a structured
>   * @frame of the HDMI Audio information frame.
> @@ -1117,11 +1126,14 @@ static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
>   * Returns 0 on success or a negative error code on failure.
>   */
>  static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
> -				       const void *buffer)
> +				       const void *buffer, size_t size)
>  {
>  	const u8 *ptr = buffer;
>  	int ret;
>  
> +	if (size < HDMI_INFOFRAME_SIZE(AUDIO))
> +		return -EINVAL;
> +
>  	if (ptr[0] != HDMI_INFOFRAME_TYPE_AUDIO ||
>  	    ptr[1] != 1 ||
>  	    ptr[2] != HDMI_AUDIO_INFOFRAME_SIZE) {
> @@ -1151,8 +1163,9 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
>  
>  /**
>   * hdmi_vendor_infoframe_unpack() - unpack binary buffer to a HDMI vendor infoframe
> - * @buffer: source buffer
>   * @frame: HDMI Vendor infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
>   *
>   * Unpacks the information contained in binary @buffer into a structured
>   * @frame of the HDMI Vendor information frame.
> @@ -1163,7 +1176,7 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
>   */
>  static int
>  hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> -				 const void *buffer)
> +				 const void *buffer, size_t size)
>  {
>  	const u8 *ptr = buffer;
>  	size_t length;
> @@ -1171,6 +1184,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>  	u8 hdmi_video_format;
>  	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
>  
> +	if (size < HDMI_INFOFRAME_HEADER_SIZE)
> +		return -EINVAL;
> +

This check is not needed since that is already done in hdmi_infoframe_unpack().

>  	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
>  	    ptr[1] != 1 ||
>  	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
> @@ -1178,6 +1194,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>  
>  	length = ptr[2];
>  
> +	if (size < HDMI_INFOFRAME_HEADER_SIZE + length)
> +		return -EINVAL;
> +
>  	if (hdmi_infoframe_checksum(buffer,
>  				    HDMI_INFOFRAME_HEADER_SIZE + length) != 0)
>  		return -EINVAL;
> @@ -1224,8 +1243,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>  
>  /**
>   * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe
> - * @buffer: source buffer
>   * @frame: HDMI infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
>   *
>   * Unpacks the information contained in binary buffer @buffer into a structured
>   * @frame of a HDMI infoframe.
> @@ -1235,23 +1255,26 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>   * Returns 0 on success or a negative error code on failure.
>   */
>  int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
> -			  const void *buffer)
> +			  const void *buffer, size_t size)
>  {
>  	int ret;
>  	const u8 *ptr = buffer;
>  
> +	if (size < HDMI_INFOFRAME_HEADER_SIZE)
> +		return -EINVAL;
> +
>  	switch (ptr[0]) {
>  	case HDMI_INFOFRAME_TYPE_AVI:
> -		ret = hdmi_avi_infoframe_unpack(&frame->avi, buffer);
> +		ret = hdmi_avi_infoframe_unpack(&frame->avi, buffer, size);
>  		break;
>  	case HDMI_INFOFRAME_TYPE_SPD:
> -		ret = hdmi_spd_infoframe_unpack(&frame->spd, buffer);
> +		ret = hdmi_spd_infoframe_unpack(&frame->spd, buffer, size);
>  		break;
>  	case HDMI_INFOFRAME_TYPE_AUDIO:
> -		ret = hdmi_audio_infoframe_unpack(&frame->audio, buffer);
> +		ret = hdmi_audio_infoframe_unpack(&frame->audio, buffer, size);
>  		break;
>  	case HDMI_INFOFRAME_TYPE_VENDOR:
> -		ret = hdmi_vendor_any_infoframe_unpack(&frame->vendor, buffer);
> +		ret = hdmi_vendor_any_infoframe_unpack(&frame->vendor, buffer, size);
>  		break;
>  	default:
>  		ret = -EINVAL;
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index d3816170c062..a577d4ae2570 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -333,7 +333,7 @@ union hdmi_infoframe {
>  ssize_t
>  hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size);
>  int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
> -			  const void *buffer);
> +			  const void *buffer, size_t size);
>  void hdmi_infoframe_log(const char *level, struct device *dev,
>  			union hdmi_infoframe *frame);
>  
> 

FYI: I'll be posting two other hdmi.c patches today that we (Cisco) have in our queue
for some time.

Regards,

	Hans

Re: [PATCH 10/10] video/hdmi: Pass buffer size to infoframe unpack functions

[Ville Syrjälä]

On Mon, Nov 20, 2017 at 02:36:20PM +0100, Hans Verkuil wrote:
> On 11/13/2017 06:04 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
<snip>
> > @@ -1163,7 +1176,7 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
> >   */
> >  static int
> >  hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> > -				 const void *buffer)
> > +				 const void *buffer, size_t size)
> >  {
> >  	const u8 *ptr = buffer;
> >  	size_t length;
> > @@ -1171,6 +1184,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> >  	u8 hdmi_video_format;
> >  	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
> >  
> > +	if (size < HDMI_INFOFRAME_HEADER_SIZE)
> > +		return -EINVAL;
> > +
> 
> This check is not needed since that is already done in hdmi_infoframe_unpack().

Hmm. True. Somehow I was expecting that this function would have been
exported on its own, but it's static so clearly I was mistaken.

The pack functions are individually exported, which is where I got
this idea probably.

> 
> >  	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
> >  	    ptr[1] != 1 ||
> >  	    (ptr[2] != 4 && ptr[2] != 5 && ptr[2] != 6))
> > @@ -1178,6 +1194,9 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> >  
> >  	length = ptr[2];
> >  
> > +	if (size < HDMI_INFOFRAME_HEADER_SIZE + length)
> > +		return -EINVAL;
> > +
> >  	if (hdmi_infoframe_checksum(buffer,
> >  				    HDMI_INFOFRAME_HEADER_SIZE + length) != 0)
> >  		return -EINVAL;

-- 
Ville Syrjälä
Intel OTC