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 for Android: free password hash cracker in your pocket

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

Message-ID: <87cyjkpcik.fsf@intel.com>
Date: Mon, 28 Oct 2024 15:45:07 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@...il.com>,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
 tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, "Syrjala, Ville"
 <ville.syrjala@...el.com>
Cc: 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 *

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.)

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?

BR,
Jani.

-- 
Jani Nikula, Intel