| Message ID | 20230109223110.1165433-1-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE | expand |
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)
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
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
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
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
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
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.
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
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)
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(-)