[PATCH 1/2] drm/amdgpu: properly handle vbios fake edid sizing

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Thomas Weißschuh <linux at weissschuh.net>
> Sent: Tuesday, July 23, 2024 1:58 PM
> To: Deucher, Alexander <Alexander.Deucher at amd.com>
> Cc: amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/amdgpu: properly handle vbios fake edid sizing
>
> On 2024-07-23 13:33:56+0000, Alex Deucher wrote:
> > The comment in the vbios structure says:
> > // = 128 means EDID length is 128 bytes, otherwise the EDID length =
> > ucFakeEDIDLength*128
> >
> > This fake edid struct has not been used in a long time, so I'm not
> > sure if there were actually any boards out there with a non-128 byte
> > EDID, but align the code with the comment.
> >
> > Reported-by: Thomas Weißschuh <linux at weissschuh.net>
>
> Afaik Reported-by: should also have a Link:.
>
> And IMO a Fixes: would also be fitting.

I can add those.

>
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >  .../gpu/drm/amd/amdgpu/atombios_encoders.c    | 24 +++++++++++------
> --
> >  1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > index 25feab188dfe..a8751a5901c6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > @@ -2065,12 +2065,16 @@
> amdgpu_atombios_encoder_get_lcd_info(struct amdgpu_encoder *encoder)
> >                                     fake_edid_record =
> (ATOM_FAKE_EDID_PATCH_RECORD *)record;
> >                                     if (fake_edid_record-
> >ucFakeEDIDLength) {
> >                                             struct edid *edid;
> > -                                           int edid_size =
> > -
>       max((int)EDID_LENGTH, (int)fake_edid_record->ucFakeEDIDLength);
> > -                                           edid = kmalloc(edid_size,
> GFP_KERNEL);
> > +                                           int edid_size;
> > +
> > +                                           if (fake_edid_record-
> >ucFakeEDIDLength == 128)
> > +                                                   edid_size =
> fake_edid_record->ucFakeEDIDLength;
> > +                                           else
> > +                                                   edid_size =
> fake_edid_record->ucFakeEDIDLength * 128;
> > +                                           edid =
> kmalloc(max(EDID_LENGTH, edid_size), GFP_KERNEL);
>
> This looks wrong, but it was similar before.
> If the EDID contains extensions and the code tries to access those, it will be an
> out of bounds memory access, because the extensions were not actually
> copied.
> (And this seems to already happen in drm_edid_is_valid())
>
> This code will go away soon with my patch, but at least we can now figure out
> what to do on EDIDs with extensions instead of changing the behaviour just as
> a side-effect.
>
> Is there any issue with just dropping the max() and keeping the full EDID?

Yes, we can drop the max().  Although I'm not entirely sure what the problem is that you are getting at.  We always copy the entire EDID from the vbios.  The EDID from the bios is either 128 bytes or a multiple of 128 bytes.  The max was only there to make sure we allocated a minimum of EDID_LENGTH bytes.

>
> Also if this is touched anyways, it could become kmemdup().

Will fix that up.

Thanks,

Alex

>
> >                                             if (edid) {
> >                                                     memcpy((u8 *)edid,
> (u8 *)&fake_edid_record->ucFakeEDIDString[0],
> > -                                                          fake_edid_record-
> >ucFakeEDIDLength);
> > +                                                          edid_size);
> >
> >                                                     if
> (drm_edid_is_valid(edid)) {
> >                                                             adev-
> >mode_info.bios_hardcoded_edid = edid; @@ -2078,13
> > +2082,13 @@ amdgpu_atombios_encoder_get_lcd_info(struct
> amdgpu_encoder *encoder)
> >                                                     } else
> >                                                             kfree(edid);
> >                                             }
> > +                                           record +=
> struct_size(fake_edid_record,
> > +
> ucFakeEDIDString,
> > +                                                                 edid_size);
> > +                                   } else {
> > +                                           /* empty fake edid record
> must be 3 bytes long */
> > +                                           record +=
> sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1;
> >                                     }
> > -                                   record += fake_edid_record-
> >ucFakeEDIDLength ?
> > -                                             struct_size(fake_edid_record,
> > -                                                         ucFakeEDIDString,
> > -                                                         fake_edid_record-
> >ucFakeEDIDLength) :
> > -                                             /* empty fake edid record
> must be 3 bytes long */
> > -
> sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1;
> >                                     break;
> >                             case
> LCD_PANEL_RESOLUTION_RECORD_TYPE:
> >                                     panel_res_record =
> (ATOM_PANEL_RESOLUTION_PATCH_RECORD *)record;
> > --
> > 2.45.2
> >