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

Message-ID: <87plw5vfdw.fsf@intel.com>
Date: Fri, 08 Mar 2024 00:36:11 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Hsin-Yi Wang <hsinyi@...omium.org>
Cc: Doug Anderson <dianders@...omium.org>, 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

On Thu, 07 Mar 2024, Hsin-Yi Wang <hsinyi@...omium.org> wrote:
> On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <jani.nikula@...ux.intel.com> wrote:
>>
>> On Wed, 06 Mar 2024, Doug Anderson <dianders@...omium.org> wrote:
>> > Hi,
>> >
>> > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@...omium.org> wrote:
>> >>
>> >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@...omium.org> wrote:
>> >> >
>> >> > 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.data.str.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'm also not sure about \0... based on
>> >> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
>> >> they use \n as terminator. Maybe we should also reject \0 before\n?
>> >> Since it's not printable.
>> >
>> > Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
>> > I guess in that case I'd prefer simply removing the code to handle
>> > '\0' instead of treating it like space until we see some actual need
>> > for it. So just get rid of the "!desc[i]" case?
>>
>> The spec text, similar for both EDID_DETAIL_MONITOR_NAME and
>> EDID_DETAIL_MONITOR_STRING:
>>
>>         Up to 13 alphanumeric characters (using ASCII codes) may be used
>>         to define the model name of the display product. The data shall
>>         be sequenced such that the 1st byte (ASCII code) = the 1st
>>         character, the 2nd byte (ASCII code) = the 2nd character,
>>         etc. If there are less than 13 characters in the string, then
>>         terminate the display product name string with ASCII code ‘0Ah’
>>         (line feed) and pad the unused bytes in the field with ASCII
>>         code ‘20h’ (space).
>>
>> In theory, only checking for '\n' for termination should be enough, and
>> this is what drm_edid_get_monitor_name() does. If there's a space
>> *before* that, it should be considered part of the name, and not
>> ignored. (So my suggestion in reply to the previous version is wrong.)
>>
>> However, since the match name uses NUL termination, maybe we should
>> ignore NULs *before* '\n'? Like so:
>>
>> for (i = name_len; i < desc_len; i++) {
>>         if (desc[i] == '\n')
>>                 break;
>>         if (!desc[i])
>>                 return;
>> }
>>
> Allow trailing white spaces so we don't need to add the trailing white
> space in edp_panel_entry.

Just so it's clear here too: Agreed.

>
> https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > -Doug
>>
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel