Re: [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 02.04.25 um 15:16 schrieb Patrik Jakobsson:
> On Tue, Apr 1, 2025 at 6:23 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> There need to be least 5 bytes in the vendor descriptor. Return
>> an error otherwise. Also change the branching to early-out on
>> the error. Adjust indention of the rest of the parser function.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/udl/udl_main.c | 72 +++++++++++++++++-----------------
>>   1 file changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>> index 4291ddb7158c4..58d6065589d3a 100644
>> --- a/drivers/gpu/drm/udl/udl_main.c
>> +++ b/drivers/gpu/drm/udl/udl_main.c
>> @@ -45,43 +45,43 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
>>                  goto unrecognized;
>>          len = ret;
>>
>> -       if (len > 5) {
>> -               DRM_INFO("vendor descriptor length: %u data:%11ph\n",
>> -                        len, desc);
>> -
>> -               if ((desc[0] != len) ||    /* descriptor length */
>> -                   (desc[1] != 0x5f) ||   /* vendor descriptor type */
>> -                   (desc[2] != 0x01) ||   /* version (2 bytes) */
>> -                   (desc[3] != 0x00) ||
>> -                   (desc[4] != len - 2))  /* length after type */
>> -                       goto unrecognized;
>> -
>> -               desc_end = desc + len;
>> -               desc += 5; /* the fixed header we've already parsed */
>> -
>> -               while (desc < desc_end) {
>> -                       u8 length;
>> -                       u16 key;
>> -
>> -                       key = le16_to_cpu(*((u16 *) desc));
>> -                       desc += sizeof(u16);
>> -                       length = *desc;
>> -                       desc++;
>> -
>> -                       switch (key) {
>> -                       case 0x0200: { /* max_area */
>> -                               u32 max_area;
>> -                               max_area = le32_to_cpu(*((u32 *)desc));
>> -                               DRM_DEBUG("DL chip limited to %d pixel modes\n",
>> -                                       max_area);
>> -                               udl->sku_pixel_limit = max_area;
>> -                               break;
>> -                       }
>> -                       default:
>> -                               break;
>> -                       }
>> -                       desc += length;
>> +       if (len < 5)
> Hi Thomas,
>
> Shouldn't this be if (len <= 5)? The old code only parsed if the
> descriptor returned at least 6 bytes.

Right, I also noticed that. But I though it was a mistake. The header is 
5 bytes and if there are no key-value pairs it's still a valid 
descriptor AFAICT. Patch 9 of the series sets a default for the pixel 
limit and the adapter would be usable. I rather not change the new 
logic, but add an explanation to the commit description. Ok?

Best regards
Thomas

>
> -Patrik
>
>> +               goto unrecognized;
>> +
>> +       DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
>> +
>> +       if ((desc[0] != len) ||    /* descriptor length */
>> +           (desc[1] != 0x5f) ||   /* vendor descriptor type */
>> +           (desc[2] != 0x01) ||   /* version (2 bytes) */
>> +           (desc[3] != 0x00) ||
>> +           (desc[4] != len - 2))  /* length after type */
>> +               goto unrecognized;
>> +
>> +       desc_end = desc + len;
>> +       desc += 5; /* the fixed header we've already parsed */
>> +
>> +       while (desc < desc_end) {
>> +               u8 length;
>> +               u16 key;
>> +
>> +               key = le16_to_cpu(*((u16 *)desc));
>> +               desc += sizeof(u16);
>> +               length = *desc;
>> +               desc++;
>> +
>> +               switch (key) {
>> +               case 0x0200: { /* max_area */
>> +                       u32 max_area = le32_to_cpu(*((u32 *)desc));
>> +
>> +                       DRM_DEBUG("DL chip limited to %d pixel modes\n",
>> +                                 max_area);
>> +                       udl->sku_pixel_limit = max_area;
>> +                       break;
>> +               }
>> +               default:
>> +                       break;
>>                  }
>> +               desc += length;
>>          }
>>
>>          goto success;
>> --
>> 2.49.0
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)