Re: [PATCH v2 2/2] drm/amd/display: Use drm_edid_block_count() instead of raw extensions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Kernel heap out-of-bounds read due to mismatch between HF-EEODB block count and raw `edid->extensions` byte.

Pre-existing issues:
- [Critical] Heap buffer overflow in amdgpu_dm_connector_funcs_force() when copying EDID.
--

--- Patch [2]: [PATCH v2 2/2] drm/amd/display: Use drm_edid_block_count() instead of raw extensions ---
commit bb5e357c1182326e11f3ce4e756df91c3df854a6
Author: Timoyoungster <timo.proemer04@gmail.com>

drm/amd/display: Use drm_edid_block_count() instead of raw extensions

Instead of manually calculating the EDID block count by reading the
extensions field from the raw edid structure (edid->extensions + 1),
utilize the core DRM helper drm_edid_block_count().

This now includes possible HF-EEODB extension blocks, which are not
included in edid->extensions and were previously truncated with the
memmove.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 71e2627f9a9d4..82f3cce870a3c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -1220,11 +1221,15 @@ enum dc_edid_status dm_helpers_read_local_edid(
>  			continue;
>  
>  		edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> -		if (!edid ||
> -		    edid->extensions >= sizeof(sink->dc_edid.raw_edid) / EDID_LENGTH)
> +		if (!edid)
>  			return EDID_BAD_INPUT;
>  
> -		sink->dc_edid.length = EDID_LENGTH * (edid->extensions + 1);
> +		block_count = drm_edid_block_count(drm_edid);
> +
> +		if (block_count > sizeof(sink->dc_edid.raw_edid) / EDID_LENGTH)
> +			return EDID_BAD_INPUT;
> +
> +		sink->dc_edid.length = EDID_LENGTH * block_count;
>  		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);

[Severity: High]
Does this create an out-of-bounds read when the copied EDID is parsed downstream?

If a malicious display provides an EDID with an extension count of 255 but an
HF-EEODB block specifying a count of 2, drm_edid_block_count() will return 2.
The bounds check passes, and memmove() copies 256 bytes into raw_edid.

However, the raw extensions byte in the copied data remains 255.
When this is later parsed in dm_helpers_parse_edid_caps() which delegates to
drm_edid_is_valid(), doesn't that legacy helper use the unmodified extensions
byte to iterate up to 256 times, reading past the end of the 2048-byte
raw_edid array on the kernel heap?

Should the raw extensions byte in the copied buffer be manually updated to match
block_count - 1, preventing legacy functions from performing out-of-bounds
reads?

Also, looking at related EDID copying code in the same subsystem:

[Severity: Critical]
This isn't a bug introduced by this patch, but does the following code in
amdgpu_dm_connector.c:amdgpu_dm_connector_funcs_force() have a heap buffer
overflow?

amdgpu_dm_connector_funcs_force() {
    ...
    memmove(dc_em_sink->dc_edid.raw_edid, edid, (edid->extensions + 1) * EDID_LENGTH);
    ...
}

If a malicious display or I2C emulator provides an EDID with extensions set to
255 and valid block checksums, and userspace with DRM privileges forces a
connector status update, it appears this could copy up to 32KB into the 2KB
dc_em_sink->dc_edid.raw_edid buffer, corrupting the heap.

Is a bounds check needed here similar to the one performed in
dm_helpers_read_local_edid()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713193237.2639-1-timo.proemer04@gmail.com?part=2