* Re: [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid [not found] <adinpZORBkhVcw31@stanley.mountain> @ 2026-04-10 12:00 ` Joshua Peisach 2026-04-11 5:30 ` SHANMUGAM, SRINIVASAN 0 siblings, 1 reply; 3+ messages in thread From: Joshua Peisach @ 2026-04-10 12:00 UTC (permalink / raw) To: Dan Carpenter, Joshua Peisach, Alex Deucher Cc: amd-gfx, SHANMUGAM, SRINIVASAN On Fri Apr 10, 2026 at 3:32 AM EDT, Dan Carpenter wrote: > > 1057 amdgpu_connector->use_digital = > --> 1058 drm_edid_is_digital(amdgpu_connector->edid); > ^^^^^^^^^^^^^^^^^^^^^^ > Use after free. > Lovely. I was wondering if that Claude review[1] was accurate. I also asked in IRC if it was something to be considered about but I didn't get a response[2]. I'll be more pressing next time, sorry. This morning I'm unable to test, but I think reverting the commit that removed amdgpu_connector_free_edid[3] should fix it. What do you think? Thanks again for the report. [1]: https://lore.gitlab.freedesktop.org/drm-ai-reviews/review-overall-20260303211823.76631-1-jpeisach@ubuntu.com/ [2]: https://people.freedesktop.org/~cbrill/dri-log/?channel=radeon&highlight_names=&date=2026-03-17&show_html=true [3]: https://gitlab.freedesktop.org/agd5f/linux/-/commit/289479173fb538f3ec7aac5206f49e39367ee75c -Josh ^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid 2026-04-10 12:00 ` [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid Joshua Peisach @ 2026-04-11 5:30 ` SHANMUGAM, SRINIVASAN 2026-04-11 11:45 ` Joshua Peisach 0 siblings, 1 reply; 3+ messages in thread From: SHANMUGAM, SRINIVASAN @ 2026-04-11 5:30 UTC (permalink / raw) To: Joshua Peisach, Dan Carpenter, Deucher, Alexander Cc: amd-gfx@lists.freedesktop.org, Wentland, Harry, Li, Roman, Hung, Alex, Chung, ChiaHsuan (Tom), Pillai, Aurabindo [Public] > -----Original Message----- > From: Joshua Peisach <jpeisach@ubuntu.com> > Sent: Friday, April 10, 2026 5:30 PM > To: Dan Carpenter <error27@gmail.com>; Joshua Peisach > <jpeisach@ubuntu.com>; Deucher, Alexander <Alexander.Deucher@amd.com> > Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN > <SRINIVASAN.SHANMUGAM@amd.com> > Subject: Re: [bug report] drm/amdgpu/amdgpu_connectors: remove > amdgpu_connector_free_edid > > On Fri Apr 10, 2026 at 3:32 AM EDT, Dan Carpenter wrote: > > > > 1057 amdgpu_connector->use_digital = > > --> 1058 drm_edid_is_digital(amdgpu_connector->edid); > > > > ^^^^^^^^^^^^^^^^^^^^^^ Use after free. > > > > Lovely. I was wondering if that Claude review[1] was accurate. I also asked in IRC if > it was something to be considered about but I didn't get a response[2]. I'll be more > pressing next time, sorry. > > This morning I'm unable to test, but I think reverting the commit that removed > amdgpu_connector_free_edid[3] should fix it. > > What do you think? I went through the code path, and the warning looks valid: In amdgpu_connector_dvi_detect(), we do: drm_edid_free(amdgpu_connector->edid); After that, we call: amdgpu_connector_get_edid(connector); But inside amdgpu_connector_get_edid(): It immediately returns if amdgpu_connector->edid is non-NULL Since we did not set amdgpu_connector->edid = NULL after freeing: The pointer is still non-NULL (but already freed) So amdgpu_connector_get_edid() becomes a no-op No new EDID is read Then later we do: drm_edid_is_digital(amdgpu_connector->edid); At this point: amdgpu_connector->edid still points to freed memory So this becomes a real use-after-free So the issue is not just the removal of amdgpu_connector_free_edid(), but that we lost the behavior of clearing the cached EDID pointer after free. Because of this, the EDID cache logic breaks. About reverting: Reverting the commit would fix it indirectly But I think a minimal fix is better: Set amdgpu_connector->edid = NULL after drm_edid_free() This keeps the current design intact and fixes the bug cleanly. Also, I noticed similar patterns in VGA and shared DDC paths, so we may want to fix those as well for consistency. I also checked amdgpu_connector_vga_detect() and amdgpu_connector_shared_ddc(). They show the same stale cached EDID pattern: amdgpu_connector->edid is freed without being cleared. In vga_detect(), this is the same functional bug as DVI because the code then calls amdgpu_connector_get_edid() and later uses amdgpu_connector->edid. In shared_ddc(), it may not trigger an immediate use-after-free in that function, but leaving the freed EDID cached is still unsafe and should be fixed for consistency. Does this approach look good? Happy to hear thoughts from others I can send a patch if this makes sense. Thanks! Srini ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid 2026-04-11 5:30 ` SHANMUGAM, SRINIVASAN @ 2026-04-11 11:45 ` Joshua Peisach 0 siblings, 0 replies; 3+ messages in thread From: Joshua Peisach @ 2026-04-11 11:45 UTC (permalink / raw) To: SHANMUGAM, SRINIVASAN, Dan Carpenter, Deucher, Alexander Cc: amd-gfx@lists.freedesktop.org, Wentland, Harry, Li, Roman, Hung, Alex, Chung, ChiaHsuan (Tom), Pillai, Aurabindo On Sat Apr 11, 2026 at 1:30 AM EDT, SRINIVASAN SHANMUGAM wrote: > I went through the code path, and the warning looks valid: > > In amdgpu_connector_dvi_detect(), we do: > drm_edid_free(amdgpu_connector->edid); > After that, we call: > amdgpu_connector_get_edid(connector); > But inside amdgpu_connector_get_edid(): > It immediately returns if amdgpu_connector->edid is non-NULL > Since we did not set amdgpu_connector->edid = NULL after freeing: > The pointer is still non-NULL (but already freed) > So amdgpu_connector_get_edid() becomes a no-op > No new EDID is read > Then later we do: > drm_edid_is_digital(amdgpu_connector->edid); > At this point: > amdgpu_connector->edid still points to freed memory > So this becomes a real use-after-free > > So the issue is not just the removal of amdgpu_connector_free_edid(), > but that we lost the behavior of clearing the cached EDID pointer after free. > > Because of this, the EDID cache logic breaks. > > About reverting: > > Reverting the commit would fix it indirectly > But I think a minimal fix is better: > Set amdgpu_connector->edid = NULL after drm_edid_free() > > This keeps the current design intact and fixes the bug cleanly. > I like it - less is more :) and it also makes it clear that the pointer is set to NULL, instead of being hidden behind a function. -Josh ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-11 11:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <adinpZORBkhVcw31@stanley.mountain>
2026-04-10 12:00 ` [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid Joshua Peisach
2026-04-11 5:30 ` SHANMUGAM, SRINIVASAN
2026-04-11 11:45 ` Joshua Peisach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox