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