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 <[email protected]>
---
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
On Mon, 09 Jan 2023, Martin Blumenstingl <[email protected]> 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 <[email protected]>
> ---
> 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
Hello Jani, Hello Ville,
On Tue, Jan 10, 2023 at 7:20 PM Jani Nikula <[email protected]> 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
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 <[email protected]>
> ---
> 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
Hello Ville.
On Mon, Feb 6, 2023 at 10:58 AM Ville Syrjälä
<[email protected]> 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
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?
> <[email protected]> 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
On Mon, Feb 13, 2023 at 12:11 PM Ville Syrjälä
<[email protected]> 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
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?
> <[email protected]> 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
On Tue, Feb 14, 2023 at 10:35 PM Ville Syrjälä
<[email protected]> 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