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

[Thomas Weißschuh]

On 2024-07-23 20:30:08+0000, Deucher, Alexander wrote:
> > -----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.

Somehow I got confused by max() and min(), so please ignore.