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 <[email protected]> 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/[email protected]?part=2
