linux-kernel - Re: [PATCH] drm/amdgpu: convert bios_hardcoded_edid to drm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sun, 16 Jun 2024 20:14:45 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Alex Deucher <alexander.deucher@....com>, 
	Christian König <christian.koenig@....com>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/amdgpu: convert bios_hardcoded_edid to drm_edid

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 <linux@...ssschuh.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>