linux-kernel - Re: [PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJvTgAj=fe4-JuBRUEHuw9jUm-SRvuxsfuSpkBfRpdU2i+rXmw@mail.gmail.com>
Date: Wed, 13 Nov 2024 21:59:36 +0530
From: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@...il.com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>, maarten.lankhorst@...ux.intel.com, 
	mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, 
	"Syrjala, Ville" <ville.syrjala@...el.com>, skhan@...uxfoundation.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use
 struct cea_db *

Thanks for the feedback.

On Mon, Oct 28, 2024 at 8:09 PM Ville Syrjälä
<ville.syrjala@...ux.intel.com> wrote:
>
> On Mon, Oct 28, 2024 at 03:45:07PM +0200, Jani Nikula wrote:
> > On Sun, 27 Oct 2024, Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@...il.com> wrote:
> > > @@ -6320,19 +6321,20 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> > >
> > >  /* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
> > >  static void
> > > -drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
> > > +drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *db)
> > >  {
> > >     struct drm_display_info *info = &connector->display_info;
> > >     u8 len = cea_db_payload_len(db);
> > > +   const u8 *data = cea_db_data(db);
> > >
> > >     info->is_hdmi = true;
> > >
> > > -   info->source_physical_address = (db[4] << 8) | db[5];
> > > +   info->source_physical_address = (data[3] << 8) | data[4];
> > >
> > >     if (len >= 6)
> > > -           info->dvi_dual = db[6] & 1;
> > > +           info->dvi_dual = data[5] & 1;
> >
> > Just commenting on one hunk, because it's a good example of the whole
> > series I think.
> >
> > The above is nice, because it improves the offset vs. length
> > comparisons. Many of the old checks like above look like off-by-ones,
> > when indexing from the beginning of the data block, not from the
> > beginning of payload, and cea_db_payload_len() excludes the first byte.
> >
> > The main problem is that the specs are written with indexing from the
> > beginning of the data block. For example, HDMI 1.4 table 8-16 defining
> > the HDMI VSDB says source physical address is at byte offsets 4 and 5,
> > and dvi dual flag at byte offset 6. That will no longer be the case in
> > code. It gets tricky to review when you have to keep adjusting the
> > offsets in your head. (I don't remember if there are specs that specify
> > the offsets starting from the "actual" payload after all the meta stuff
> > has been removed.)
>
> IIRC there was some off-by-one indexing difference between
> some of the specs. But I don't remember which ones use what.
>
> >
> > Now, if we accept having to do that mental acrobatics, why stop there?
> > You also have extended tags (first payload byte is the tag), as well as
> > vendor tags (first three payload bytes are the OUI). It begs the
> > question whether there should be higher level data and length helpers
> > that identify and remove the tags (including extended tags and OUI
> > stuff). For example, the actual data for HDMI VSDB starts at payload
> > offset 3, as the first three bytes are the HDMI OUI.
> >
> > What to do? Ville, thoughts?
>
> So just different *_{data,len}() for the different indexing variants
> (as defined by the relevant spec)? That seems like a reasonable
> apporach as then the len vs. index checks might actually make sense.
>
Please let me know if the below snippet matches the feedback.

const u8 *vsdb_data = cea_db_to_vsdb_data(db); // skips indexes for HDMI OUI
...
info->source_physical_address = (vsdb_data[0] << 8) | vsdb_data[1];
...
info->dvi_dual = vsdb_data[2] & 1;
> --
> Ville Syrjälä
> Intel

Thanks,
Vamsi