Hi amdgpu maintainers,
did you get a chance to look at the patch and the suspected bug?
Thanks,
Thomas
On 2024-06-16 20:14:45+0000, Thomas Weißschuh wrote:
> On 2024-06-16 11:12:03+0000, Thomas Weißschuh wrote:
> > Instead of manually passing around 'struct edid *' and its size,
> > use 'struct drm_edid', which encapsulates a validated combination of
> > both.
> >
> > As the drm_edid_ can handle NULL gracefully, the explicit checks can be
> > dropped.
> >
> > Also save a few characters by transforming '&array[0]' to the equivalent
> > 'array' and using 'max_t(int, ...)' instead of manual casts.
> >
> > Signed-off-by: Thomas Weißschuh <li...@weissschuh.net>
> > ---
> > While this patch introduces a new user for drm_edid_raw(),
> > if amdgpu proper gets migrated to 'struct drm_edid', that usage will go
> > away.
> >
> > This is only compile-tested.
> >
> > I have some more patches for the rest of amdgpu,
> > to move to 'struct drm_edid'.
> > This patch is a test-balloon for the general idea.
> >
> > The same can also be done for drm/radeon.
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 6 +-----
> > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 4 ++--
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 21 +++++++--------------
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> > 8 files changed, 15 insertions(+), 26 deletions(-)
>
> <snip>
>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > index 25feab188dfe..90383094ed1e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > @@ -2064,20 +2064,13 @@ amdgpu_atombios_encoder_get_lcd_info(struct
> > amdgpu_encoder *encoder)
> > case LCD_FAKE_EDID_PATCH_RECORD_TYPE:
> > 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);
> > - if (edid) {
> > - memcpy((u8 *)edid, (u8
> > *)&fake_edid_record->ucFakeEDIDString[0],
> > -
> > fake_edid_record->ucFakeEDIDLength);
> > -
> > - if
> > (drm_edid_is_valid(edid)) {
> > -
> > adev->mode_info.bios_hardcoded_edid = edid;
> > -
> > adev->mode_info.bios_hardcoded_edid_size = edid_size;
> > - } else
> > - kfree(edid);
> > - }
> > + const struct drm_edid *edid;
> > + edid =
> > drm_edid_alloc(fake_edid_record->ucFakeEDIDString,
> > +
> > max_t(int, EDID_LENGTH, fake_edid_record->ucFakeEDIDLength));
> > + if (drm_edid_valid(edid))
> > +
> > adev->mode_info.bios_hardcoded_edid = edid;
> > + else
> > + drm_edid_free(edid);
>
> The old code here seems broken in general.
> In drivers/gpu/drm/amd/include/atombios.h the comment for ucFakeEDIDLength
> says:
> (I expect the same field in the same struct for amdgpu to have the same
> semantics)
>
> UCHAR ucFakeEDIDLength; // = 128 means EDID length is 128 bytes,
> otherwise the EDID length = ucFakeEDIDLength*128
>
> So as soon as the EDID from the BIOS has extensions, only the first few
> bytes will be copied into the allocated memory. drm_edid_is_valid() will
> then read the uninitialized memory and if the "extensions" field ends up
> non-zero it will happily "validate" past the allocated buffer.
>
> The new code won't work either but at least it won't read uninitialized
> memory nor will it read past the buffer bounds.
>
> > }
> > record +=
> > fake_edid_record->ucFakeEDIDLength ?
> > struct_size(fake_edid_record,
>
> <snip>
