Re: [PATCH 16/24] 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:
- [High] Lockless modification of `connector->edid_blob_ptr` in HPD handlers.
--

--- Patch [16]: [PATCH 16/24] drm/msm/dp: remove cached drm_edid from panel ---
commit 379cd36eb86a5e848dbe71b567a862219cda4dab
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
Subject: drm/msm/dp: remove cached drm_edid from panel

This commit removes the cached drm_edid from the DP panel structure.
Instead of caching it, the EDID is read and updated directly in the plug
stage to simplify the code.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -272,6 +272,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	const struct drm_display_info *info = &connector->display_info;
>  	int rc = 0;
>  	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> +	const struct drm_edid *drm_edid = NULL;
>  
>  	rc = drm_dp_read_dpcd_caps(dp->aux, dpcd);
>  	if (rc)
> @@ -279,10 +280,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: High]
This isn't a bug introduced by this patch, but does calling
drm_edid_connector_update() here in the hotplug handler occur without holding
drm_mode_config.mutex?

If this is called from an IRQ thread holding only the driver-local
dp->plugged_lock, drm_edid_connector_update() will replace
connector->edid_blob_ptr and decrement the old blob's reference count.

Concurrently, if userspace reads the EDID via sysfs or queries modes via
ioctl, it will acquire drm_mode_config.mutex and directly dereference the
blob. Can this lead to a Use-After-Free if the EDID blob is freed while
readers are still accessing it?

[ ... ]
> @@ -456,7 +468,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: High]
This is a pre-existing issue, but a similar lockless update to the
connector's EDID property blob seems to happen here during an unplug event.

Does this unplug path also need to hold drm_mode_config.mutex to prevent a
potential Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=16