Re: [PATCH 4/8] drm/amd/display: Copy the preferred and highest refresh modes after probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amd/display: dc_sink UAF in
  amdgpu_dm_connector_update_mode_copies()
--

commit 1f5a443bde024bb59f9710748a6f559dc056a617
Author: Hugo Sales <hugo@hsal.es>

drm/amd/display: Copy the preferred and highest refresh modes after probe

This commit adds amdgpu_dm_connector_fill_modes() to copy the preferred mode,
highest refresh rate mode, and sink EDID into struct amdgpu_dm_connector
under connection_mutex after probing a connector.

Link: https://lore.kernel.org/r/auh8pUx_kXUYSdZ8@intel.com

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index e477ef61ce109..491965266b786 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c

[ ... ]

> @@ -1223,6 +1268,87 @@ bool amdgpu_dm_is_freesync_video_mode(const struct drm_display_mode *mode,
>  }
>  EXPORT_IF_KUNIT(amdgpu_dm_is_freesync_video_mode);
>  
> +STATIC_IFN_KUNIT void
> +amdgpu_dm_connector_update_mode_copies(struct amdgpu_dm_connector *aconnector)
> +{
> +	struct drm_mode_config *config = &aconnector->base.dev->mode_config;
> +	struct list_head *modes = &aconnector->base.modes;
> +	const struct drm_display_mode *preferred, *highest;
> +	struct drm_display_mode native_buf;
> +	const struct dc_sink *sink;
> +
> +	lockdep_assert_held(&config->mutex);
> +
> +	drm_modeset_lock(&config->connection_mutex, NULL);
> +
> +	sink = aconnector->dc_sink;
> +	aconnector->mode_copies_edid_size = amdgpu_dm_sink_edid_size(sink);
> +	if (aconnector->mode_copies_edid_size)
> +		memcpy(aconnector->mode_copies_edid, sink->dc_edid.raw_edid,
> +		       aconnector->mode_copies_edid_size);

[Severity: High]
Can amdgpu_dm_connector_update_mode_copies() read a freed aconnector->dc_sink
here?

In amdgpu_dm_connector_fill_modes(), drm_helper_probe_single_connector_modes()
acquires root->mst_mgr.base.lock during MST port detection and drops it on its
exit path before amdgpu_dm_connector_update_mode_copies() runs. Meanwhile,
amdgpu_dm_mst_connector_early_unregister() runs from
drm_dp_delayed_destroy_work under connector->mutex and
root->mst_mgr.base.lock, without holding mode_config.mutex or
connection_mutex, and frees dc_sink before clearing aconnector->dc_sink:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c:amdgpu_dm_mst_connector_early_unregister() {
    ...
	if (dc_sink) {
		amdgpu_dm_With_dc_lock(adev,
			dc_link_remove_remote_sink(dc_link, dc_sink));
		dc_sink_release(dc_sink);
		aconnector->dc_sink = NULL;
    ...
}

CPU0                                  CPU1 (kworker)
amdgpu_dm_connector_fill_modes()
  drm_helper_probe_single_...()
    ...
    drm_modeset_drop_locks(&ctx)
                                      drm_dp_delayed_destroy_work()
                                        ...
                                        amdgpu_dm_mst_connector_early_unregister()
                                          lock(&root->mst_mgr.base.lock)
                                          dc_sink_release(dc_sink) -> kfree()
  amdgpu_dm_connector_update_mode_copies()
    sink = aconnector->dc_sink;
    amdgpu_dm_sink_edid_size(sink)
    memcpy(..., sink->dc_edid.raw_edid, ...)  <-- use-after-free
                                          aconnector->dc_sink = NULL;

dm_resume() similarly releases and clears aconnector->dc_sink on SST
connectors under aconnector->hpd_lock without mode_config.mutex or
connection_mutex. This causes a slab use-after-free read of struct dc_sink
when copying sink->dc_edid.raw_edid.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-amdgpu-modes-four-v1-0-f11e2a6cd407@hsal.es?part=4