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

[Thomas Weißschuh]

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.

> 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?

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

>  						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
>