Re: [PATCH 03/11] drm/mediatek: mtk_dp: Fix hdmi codec and phy driver unregistration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double unregister of platform device in error paths causes Use-After-Free
- [Medium] Incorrect pointer passed to devm_add_action_or_reset causes resource leak of audio platform device
--

commit 2bc8391d481088826fa13d96d05225e5c15e6457
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dp: Fix hdmi codec and phy driver unregistration

This commit introduces devm actions to unregister the HDMI Codec and
DisplayPort PHY platform devices, preventing multiple instantiations
during probe deferrals.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index eefbc7e0f9c8d..2c738c16dafab 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c

[ ... ]

> @@ -2680,18 +2687,29 @@ static int mtk_dp_register_audio_driver(struct device *dev)
>  		.data = mtk_dp,
>  		.no_capture_mute = 1,
>  	};
> +	int ret;
>  
>  	mtk_dp->audio_pdev = platform_device_register_data(dev,
>  							   HDMI_CODEC_DRV_NAME,
>  							   PLATFORM_DEVID_AUTO,
>  							   &codec_data,
>  							   sizeof(codec_data));
> -	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);
> +	if (IS_ERR(mtk_dp->audio_pdev))
> +		return PTR_ERR(mtk_dp->audio_pdev);
> +
> +	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);

[Severity: Medium]
Is the correct platform device pointer being passed here?

This registers a cleanup action using mtk_dp->phy_dev rather than the
newly created mtk_dp->audio_pdev. This will likely leak the audio device on
probe deferral or driver unbind, and may attempt to unregister the PHY device
at the wrong time instead.

> +	if (ret) {
> +		platform_device_unregister(mtk_dp->audio_pdev);
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add codec unregister devm action");
> +	}
> +	return 0;
>  }
>  
>  static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  {
>  	struct device *dev = mtk_dp->dev;
> +	int ret;
>  
>  	mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek-dp-phy",
>  							PLATFORM_DEVID_AUTO,
> @@ -2701,6 +2719,13 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  		return dev_err_probe(dev, PTR_ERR(mtk_dp->phy_dev),
>  				     "Failed to create device mediatek-dp-phy\n");
>  
> +	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);
> +	if (ret) {
> +		platform_device_unregister(mtk_dp->phy_dev);

[Severity: High]
Could this cause a use-after-free?

When devm_add_action_or_reset() fails, it automatically executes the cleanup
action, which calls platform_device_unregister() on mtk_dp->phy_dev. Calling
platform_device_unregister() a second time here would double-free the device.

> +		return dev_err_probe(dev, ret,
> +				     "Failed to add phy unregister devm action");
> +	}
> +
>  	mtk_dp_get_calibration_data(mtk_dp);
>  
>  	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");

[Severity: High]
Can the error path for devm_phy_get() also trigger a double-free?

Looking at the rest of mtk_dp_register_phy() just below this diff:

mtk_dp_register_phy() {
    ...
    mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");
    if (IS_ERR(mtk_dp->phy)) {
        platform_device_unregister(mtk_dp->phy_dev);
        return dev_err_probe(dev, PTR_ERR(mtk_dp->phy), "Failed to get phy\n");
    }
    ...
}

Since the devm unregister action was successfully registered above, returning
an error here will trigger the devm unwind loop. That will unregister the
device a second time after this explicit platform_device_unregister() call.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701122024.19557-1-angelogioacchino.delregno@collabora.com?part=3