[bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[Dan Carpenter]

Hello Joshua Peisach,

Commit 71036457ad85 ("drm/amdgpu/amdgpu_connectors: remove
amdgpu_connector_free_edid") from Mar 3, 2026 (linux-next), leads to
the following Smatch static checker warning:

	drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:1058 amdgpu_connector_dvi_detect()
	warn: passing freed memory 'amdgpu_connector->edid' (line 1048)

drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
    1032                 /* Sometimes the pins required for the DDC probe on DVI
    1033                  * connectors don't make contact at the same time that the ones
    1034                  * for HPD do. If the DDC probe fails even though we had an HPD
    1035                  * signal, try again later
    1036                  */
    1037                 if (!dret && !force &&
    1038                     amdgpu_display_hpd_sense(adev, amdgpu_connector->hpd.hpd)) {
    1039                         DRM_DEBUG_KMS("hpd detected without ddc, retrying in 1 second\n");
    1040                         amdgpu_connector->detected_hpd_without_ddc = true;
    1041                         schedule_delayed_work(&adev->hotplug_work,
    1042                                               msecs_to_jiffies(1000));
    1043                         goto exit;
    1044                 }
    1045         }
    1046         if (dret) {
    1047                 amdgpu_connector->detected_by_load = false;
    1048                 drm_edid_free(amdgpu_connector->edid);
                                       ^^^^^^^^^^^^^^^^^^^^^^
This frees ->edid.  The old code used to set amdgpu_connector->edid = NULL
after freeing it.

    1049                 amdgpu_connector_get_edid(connector);
                                                   ^^^^^^^^^
This function call is supposed to re-assign ->edid but because it's no
longer NULL then it's just a no-op.  (It's so annoying that the naming
switches between amdgpu_connector which and connector which are basically
castings of each other).

    1050 
    1051                 if (!amdgpu_connector->edid) {
    1052                         drm_err(adev_to_drm(adev), "%s: probed a monitor but no|invalid EDID\n",
    1053                                         connector->name);
    1054                         ret = connector_status_connected;
    1055                         broken_edid = true; /* defer use_digital to later */
    1056                 } else {
    1057                         amdgpu_connector->use_digital =
--> 1058                                 drm_edid_is_digital(amdgpu_connector->edid);
                                                             ^^^^^^^^^^^^^^^^^^^^^^
Use after free.

    1059 
    1060                         /* some oems have boards with separate digital and analog connectors

This email is a free service from the Smatch-CI project [smatch.sf.net].

regards,
dan carpenter

Re: [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[Joshua Peisach]

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

RE: [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[SHANMUGAM, SRINIVASAN]

[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

Re: [bug report] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[Joshua Peisach]

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