linux-kernel - Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity

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] [thread-next>] [day] [month] [year] [list]

Date: Wed, 6 Mar 2024 15:29:44 -0800
From: Doug Anderson <dianders@...omium.org>
To: Hsin-Yi Wang <hsinyi@...omium.org>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Jessica Zhang <quic_jesszhan@...cinc.com>, Sam Ravnborg <sam@...nborg.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity

Hi,

On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@...omium.org> wrote:
>
> +static void
> +match_identity(const struct detailed_timing *timing, void *data)
> +{
> +       struct drm_edid_match_closure *closure = data;
> +       unsigned int i;
> +       const char *name = closure->ident->name;
> +       unsigned int name_len = strlen(name);
> +       const char *desc = timing->data.other_data.data.str.str;
> +       unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.datastr.str);
> +
> +       if (name_len > desc_len ||
> +           !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
> +             is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
> +               return;
> +
> +       if (strncmp(name, desc, name_len))
> +               return;
> +
> +       /* Allow trailing white spaces and \0. */
> +       for (i = name_len; i < desc_len; i++) {
> +               if (desc[i] == '\n')
> +                       break;
> +               if (!isspace(desc[i]) && !desc[i])
> +                       return;
> +       }

If my code analysis is correct, I think you'll reject the case where:

name = "foo"
desc[13] = "foo \0zzzzzzzz"

..but you'll accept these cases:

desc[13] = "foo \nzzzzzzzz"
desc[13] = "foo \0\0\0\0\0\0\0\0\0"

It somehow seems weird to me that a '\n' terminates the string but not a '\0'.

I would have done:

      for (i = name_len; i < desc_len; i++) {
              /* Consider \n or \0 to terminate the string */
              if (desc[i] == '\n' || desc[i] == '\0')
                      break;
              /* OK for spaces at the end, but non-space is a fail */
              if (!isspace(desc[i]))
                      return;
      }


> @@ -367,6 +367,12 @@ struct edid {
>         u8 checksum;
>  } __attribute__((packed));
>
> +/* EDID matching */
> +struct drm_edid_ident {
> +       u32 panel_id;
> +       const char *name;

Might not hurt to have a comment for panel_id saying that it's encoded
by drm_edid_encode_panel_id() so it's obvious what this random u32 is.


-Doug