[PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

When support for the HDMI vendor infoframe was introduced back with
commit 7d27becb3532 ("video/hdmi: Introduce helpers for the HDMI vendor
specific infoframe") it's payload size was either 5 or 6 bytes,
depending on:
  if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
When true the size was 6 bytes, otherwise 5 bytes.

Drivers that are using hdmi_infoframe_pack() are reserving 10 bytes (4
bytes for the header and up to 6 bytes for the infoframe payload data)
or more (exynos_hdmi reserves 25 bytes).

Over time the frame payload length was reduced to 4 bytes. This however
does not match the code from hdmi_hdmi_infoframe_pack() where ptr[8] and
ptr[9] are written, which means the infoframe has to allow up to 6 bytes
of payload data (considering that the header takes 4 bytes).

Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
size and avoid an out of bounds write to ptr[8] or ptr[9].

Fixes: c5e69ab35c0d ("video/hdmi: Constify infoframe passed to the pack functions")
Fixes: d43be2554b58 ("drivers: video: hdmi: cleanup coding style in video a bit")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
I'm not an expert on this topic and I'm not sure if the size still
depends on that if condition from long time ago. So please share your
thoughts.


 include/linux/hdmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 2f4dcc8d060e..026c5ef5a1a5 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -57,7 +57,7 @@ enum hdmi_infoframe_type {
 #define HDMI_SPD_INFOFRAME_SIZE    25
 #define HDMI_AUDIO_INFOFRAME_SIZE  10
 #define HDMI_DRM_INFOFRAME_SIZE    26
-#define HDMI_VENDOR_INFOFRAME_SIZE  4
+#define HDMI_VENDOR_INFOFRAME_SIZE  6
 
 #define HDMI_INFOFRAME_SIZE(type)	\
 	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
-- 
2.39.0

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Jani Nikula]

On Mon, 09 Jan 2023, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> When support for the HDMI vendor infoframe was introduced back with
> commit 7d27becb3532 ("video/hdmi: Introduce helpers for the HDMI vendor
> specific infoframe") it's payload size was either 5 or 6 bytes,
> depending on:
>   if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> When true the size was 6 bytes, otherwise 5 bytes.
>
> Drivers that are using hdmi_infoframe_pack() are reserving 10 bytes (4
> bytes for the header and up to 6 bytes for the infoframe payload data)
> or more (exynos_hdmi reserves 25 bytes).
>
> Over time the frame payload length was reduced to 4 bytes. This however
> does not match the code from hdmi_hdmi_infoframe_pack() where ptr[8] and
> ptr[9] are written, which means the infoframe has to allow up to 6 bytes
> of payload data (considering that the header takes 4 bytes).
>
> Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
> hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
> size and avoid an out of bounds write to ptr[8] or ptr[9].
>
> Fixes: c5e69ab35c0d ("video/hdmi: Constify infoframe passed to the pack functions")
> Fixes: d43be2554b58 ("drivers: video: hdmi: cleanup coding style in video a bit")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> I'm not an expert on this topic and I'm not sure if the size still
> depends on that if condition from long time ago. So please share your
> thoughts.

I tried to look at this quickly, but it makes my brain hurt. I don't
think simply changing the size here is right either.

Cc: Ville.

BR,
Jani.



>
>
>  include/linux/hdmi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 2f4dcc8d060e..026c5ef5a1a5 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -57,7 +57,7 @@ enum hdmi_infoframe_type {
>  #define HDMI_SPD_INFOFRAME_SIZE    25
>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
>  #define HDMI_DRM_INFOFRAME_SIZE    26
> -#define HDMI_VENDOR_INFOFRAME_SIZE  4
> +#define HDMI_VENDOR_INFOFRAME_SIZE  6
>  
>  #define HDMI_INFOFRAME_SIZE(type)	\
>  	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

Hello Jani, Hello Ville,

On Tue, Jan 10, 2023 at 7:20 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
[...]
> > I'm not an expert on this topic and I'm not sure if the size still
> > depends on that if condition from long time ago. So please share your
> > thoughts.
>
> I tried to look at this quickly, but it makes my brain hurt. I don't
> think simply changing the size here is right either.
I think I see what you're saying here: hdmi_vendor_infoframe_length()
has logic to determine the "correct" size.

My idea here is to use the maximum possible size for
HDMI_VENDOR_INFOFRAME_SIZE so it can be used with the
HDMI_INFOFRAME_SIZE macro (just like the other _SIZE definitions right
above the vendor infoframe one).
If you have suggestions on my patch then please let me know.


Best regards,
Martin

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Ville Syrjälä]

On Mon, Jan 09, 2023 at 11:31:10PM +0100, Martin Blumenstingl wrote:
> When support for the HDMI vendor infoframe was introduced back with
> commit 7d27becb3532 ("video/hdmi: Introduce helpers for the HDMI vendor
> specific infoframe") it's payload size was either 5 or 6 bytes,
> depending on:
>   if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> When true the size was 6 bytes, otherwise 5 bytes.
> 
> Drivers that are using hdmi_infoframe_pack() are reserving 10 bytes (4
> bytes for the header and up to 6 bytes for the infoframe payload data)
> or more (exynos_hdmi reserves 25 bytes).
> 
> Over time the frame payload length was reduced to 4 bytes. This however
> does not match the code from hdmi_hdmi_infoframe_pack() where ptr[8] and
> ptr[9] are written, which means the infoframe has to allow up to 6 bytes
> of payload data (considering that the header takes 4 bytes).
> 
> Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
> hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
> size and avoid an out of bounds write to ptr[8] or ptr[9].

The function should return -ENOSPC if the caller didn't
provide a big enough buffer. Are you saying there are
drivers that are passing a bogus size here?

> 
> Fixes: c5e69ab35c0d ("video/hdmi: Constify infoframe passed to the pack functions")
> Fixes: d43be2554b58 ("drivers: video: hdmi: cleanup coding style in video a bit")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> I'm not an expert on this topic and I'm not sure if the size still
> depends on that if condition from long time ago. So please share your
> thoughts.
> 
> 
>  include/linux/hdmi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 2f4dcc8d060e..026c5ef5a1a5 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -57,7 +57,7 @@ enum hdmi_infoframe_type {
>  #define HDMI_SPD_INFOFRAME_SIZE    25
>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
>  #define HDMI_DRM_INFOFRAME_SIZE    26
> -#define HDMI_VENDOR_INFOFRAME_SIZE  4
> +#define HDMI_VENDOR_INFOFRAME_SIZE  6
>  
>  #define HDMI_INFOFRAME_SIZE(type)	\
>  	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
> -- 
> 2.39.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

Hello Ville.

On Mon, Feb 6, 2023 at 10:58 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
[...]
> > Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
> > hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
> > size and avoid an out of bounds write to ptr[8] or ptr[9].
>
> The function should return -ENOSPC if the caller didn't
> provide a big enough buffer.
Indeed, I'm not sure why I didn't notice when I sent the patch.

> Are you saying there are drivers that are passing a bogus size here?
Thankfully not - at least when I checked the last time drivers passed
a 10 byte - or bigger - buffer.
My main concern is the HDMI_INFOFRAME_SIZE macro. It's used in various
drivers like this:
  u8 buffer[HDMI_INFOFRAME_SIZE(AVI)];

One could use HDMI_VENDOR_INFOFRAME_SIZE with this as well:
  u8 buffer[HDMI_INFOFRAME_SIZE(VENDOR)];
But it would only result in an 8 byte wide buffer.
Nobody uses it like this yet.

Do you see any reason why my patch could cause problems?
If not then I want to re-send it with an updated description.


Best regards,
Martin

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Ville Syrjälä]

On Sat, Feb 11, 2023 at 09:43:50PM +0100, Martin Blumenstingl wrote:
> Hello Ville.
> 
> On Mon, Feb 6, 2023 at 10:58 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> [...]
> > > Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
> > > hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
> > > size and avoid an out of bounds write to ptr[8] or ptr[9].
> >
> > The function should return -ENOSPC if the caller didn't
> > provide a big enough buffer.
> Indeed, I'm not sure why I didn't notice when I sent the patch.
> 
> > Are you saying there are drivers that are passing a bogus size here?
> Thankfully not - at least when I checked the last time drivers passed
> a 10 byte - or bigger - buffer.
> My main concern is the HDMI_INFOFRAME_SIZE macro. It's used in various
> drivers like this:
>   u8 buffer[HDMI_INFOFRAME_SIZE(AVI)];
> 
> One could use HDMI_VENDOR_INFOFRAME_SIZE with this as well:
>   u8 buffer[HDMI_INFOFRAME_SIZE(VENDOR)];
> But it would only result in an 8 byte wide buffer.
> Nobody uses it like this yet.

Not sure that would make any sense since a vendor
specific infoframe has no defined size until you
figure out which vendor defined it (via the OUI).

I suppose the current value of 4 is also a bit nonsense
as well then, becasue that is a legal value for the
HDMI 1.4 vendor specific infoframe, but might not be
valid for any other infoframe.

We should perhaps just get rid of HDMI_VENDOR_INFOFRAME_SIZE
entirely.

> 
> Do you see any reason why my patch could cause problems?
> If not then I want to re-send it with an updated description.
> 
> 
> Best regards,
> Martin

-- 
Ville Syrjälä
Intel

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

On Mon, Feb 13, 2023 at 12:11 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
[...]
> > One could use HDMI_VENDOR_INFOFRAME_SIZE with this as well:
> >   u8 buffer[HDMI_INFOFRAME_SIZE(VENDOR)];
> > But it would only result in an 8 byte wide buffer.
> > Nobody uses it like this yet.
>
> Not sure that would make any sense since a vendor
> specific infoframe has no defined size until you
> figure out which vendor defined it (via the OUI).
My understanding is that all of the existing HDMI vendor infoframe
code is built for HDMI_IEEE_OUI.

> I suppose the current value of 4 is also a bit nonsense
> as well then, becasue that is a legal value for the
> HDMI 1.4 vendor specific infoframe, but might not be
> valid for any other infoframe.
>
> We should perhaps just get rid of HDMI_VENDOR_INFOFRAME_SIZE
> entirely.
My thought was to make it the correct size for
drm_hdmi_vendor_infoframe_from_display_mode(). Then developers using
this "common" vendor infoframe don't have to worry much.
If there's another vendor infoframe implementation (which I'm not
aware of, but it may exist - since as you point out: it's vendor
specific) then the driver code shouldn't use
drm_hdmi_vendor_infoframe_from_display_mode() but rather implement
something custom. At that point the person implementing that will also
need to know their specific infoframe maximum size.


Best regards,
Martin

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Ville Syrjälä]

On Tue, Feb 14, 2023 at 10:26:24PM +0100, Martin Blumenstingl wrote:
> On Mon, Feb 13, 2023 at 12:11 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> [...]
> > > One could use HDMI_VENDOR_INFOFRAME_SIZE with this as well:
> > >   u8 buffer[HDMI_INFOFRAME_SIZE(VENDOR)];
> > > But it would only result in an 8 byte wide buffer.
> > > Nobody uses it like this yet.
> >
> > Not sure that would make any sense since a vendor
> > specific infoframe has no defined size until you
> > figure out which vendor defined it (via the OUI).
> My understanding is that all of the existing HDMI vendor infoframe
> code is built for HDMI_IEEE_OUI.

Only because no one has bothered to implement any
others.

> 
> > I suppose the current value of 4 is also a bit nonsense
> > as well then, becasue that is a legal value for the
> > HDMI 1.4 vendor specific infoframe, but might not be
> > valid for any other infoframe.
> >
> > We should perhaps just get rid of HDMI_VENDOR_INFOFRAME_SIZE
> > entirely.
> My thought was to make it the correct size for
> drm_hdmi_vendor_infoframe_from_display_mode(). Then developers using
> this "common" vendor infoframe don't have to worry much.
> If there's another vendor infoframe implementation (which I'm not
> aware of, but it may exist - since as you point out: it's vendor
> specific) then the driver code shouldn't use
> drm_hdmi_vendor_infoframe_from_display_mode() but rather implement
> something custom. At that point the person implementing that will also
> need to know their specific infoframe maximum size.

Yes but that other infoframe will still have
type==HDMI_INFOFRAME_TYPE_VENDOR, and
HDMI_INFOFRAME_SIZE(VENDOR) would again
give the wrong answer.

-- 
Ville Syrjälä
Intel

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

On Tue, Feb 14, 2023 at 10:35 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
[...]
> > > We should perhaps just get rid of HDMI_VENDOR_INFOFRAME_SIZE
> > > entirely.
> > My thought was to make it the correct size for
> > drm_hdmi_vendor_infoframe_from_display_mode(). Then developers using
> > this "common" vendor infoframe don't have to worry much.
> > If there's another vendor infoframe implementation (which I'm not
> > aware of, but it may exist - since as you point out: it's vendor
> > specific) then the driver code shouldn't use
> > drm_hdmi_vendor_infoframe_from_display_mode() but rather implement
> > something custom. At that point the person implementing that will also
> > need to know their specific infoframe maximum size.
>
> Yes but that other infoframe will still have
> type==HDMI_INFOFRAME_TYPE_VENDOR, and
> HDMI_INFOFRAME_SIZE(VENDOR) would again
> give the wrong answer.
So this means the way forward is to remove HDMI_VENDOR_INFOFRAME_SIZE?
That means it's up to the (HDMI) driver developers to use a big enough
buffer (by hard-coding the size). Last time I checked all drivers did.


Best regards,
Martin