[Public] > -----Original Message----- > From: Joshua Peisach <[email protected]> > Sent: Friday, April 10, 2026 5:30 PM > To: Dan Carpenter <[email protected]>; Joshua Peisach > <[email protected]>; Deucher, Alexander <[email protected]> > Cc: [email protected]; SHANMUGAM, SRINIVASAN > <[email protected]> > 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
