On 2024-07-23 20:30:08+0000, Deucher, Alexander wrote:
> > -----Original Message-----
> > From: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > Sent: Tuesday, July 23, 2024 1:58 PM
> > To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxx>
> > > ---
> > > .../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.
