Re: [PATCH v7 01/15] drm/msm/dp: remove cached drm_edid from panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unsynchronized updates to connector->edid_blob_ptr and connector->display_info leading to data races and potential Use-After-Free.
--

commit 6f73beae4d0acbc00f38ca7b1a881d309a66701f
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: remove cached drm_edid from panel

This commit simplifies the EDID handling in the MSM DP driver by removing the
cached drm_edid from the panel structure. It uses the drm_edid pointer
directly in the plug stage instead and updates the connector accordingly.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index dc6f33809ca5f..bce165bac106b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -276,10 +277,20 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  
>  	dp->link->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
>  
> -	rc = msm_dp_panel_read_sink_caps(dp->panel, connector);
> +	rc = msm_dp_panel_read_link_caps(dp->panel, connector);
>  	if (rc)
>  		goto end;
>  
> +	drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
> +	drm_edid_connector_update(connector, drm_edid);

[Severity: Critical]
This is a pre-existing issue, but does msm_dp_display_process_hpd_high()
need to hold drm_mode_config.mutex before calling
drm_edid_connector_update()?

Looking at the DRM core guidelines, connector->edid_blob_ptr and
connector->display_info are protected by drm_mode_config.mutex.

Since this runs from an unprotected IRQ worker context upon hotplug, could
an unsynchronized update cause a use-after-free? If userspace concurrently
invokes a DRM_IOCTL_MODE_GETCONNECTOR request, it might read the old blob
while it is being replaced and freed here.

> +
> +	if (!drm_edid) {
> +		DRM_ERROR("panel edid read failed\n");

[ ... ]

> @@ -453,7 +465,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
>  
>  	/* Don't forget modes for eDP */
>  	if (!dp->msm_dp_display.is_edp)
> -		msm_dp_panel_unplugged(dp->panel, dp->msm_dp_display.connector);
> +		drm_edid_connector_update(dp->msm_dp_display.connector, NULL);

[Severity: Critical]
This is a pre-existing issue, but does msm_dp_hpd_unplug_handle() similarly
need synchronization?

Clearing the EDID property without the mode config mutex might race with
other readers of the connector state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=1