Re: [PATCH AUTOSEL 6.18-5.10] drm/rockchip: dw_hdmi: Use resume_early pm ops for system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/rockchip: dw_hdmi: UAF in dw_hdmi_rockchip_resume_early() on stale drvdata
--

commit c6209b9cde57945de5a9c2033ae8339d9c3026b8
Author: Jonas Karlman <jonas@kwiboo.se>

drm/rockchip: dw_hdmi: Use resume_early pm ops for system suspend

This commit switches the dw-hdmi Rockchip glue driver to use the
resume_early PM callback so that dw_hdmi_resume() runs before
rockchip_drm_sys_resume() during system resume. It also adds a NULL check on
the driver data pointer in dw_hdmi_rockchip_resume_early().

Link: https://patch.msgid.link/20260518193748.2482823-12-jonas@kwiboo.se

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 727cdf768161a..ff34aeef3a6ff 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -635,17 +635,18 @@ static void dw_hdmi_rockchip_remove(struct platform_device *pdev)
>  	component_del(&pdev->dev, &dw_hdmi_rockchip_ops);
>  }
>  
> -static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> +static int __maybe_unused dw_hdmi_rockchip_resume_early(struct device *dev)
>  {
>  	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
>  
> -	dw_hdmi_resume(hdmi->hdmi);
> +	if (hdmi)
> +		dw_hdmi_resume(hdmi->hdmi);

[Severity: High]
Is checking if (hdmi) sufficient to guarantee that hdmi and hdmi->hdmi are
valid here?

In drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c, hdmi is allocated with
devm_kzalloc() inside dw_hdmi_rockchip_bind(), which runs inside the
component devres group opened by component_bind().

dw_hdmi_rockchip_bind() sets the driver data before calling dw_hdmi_bind(),
and does not clear it on failure:

	platform_set_drvdata(pdev, hdmi);

	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);

	if (IS_ERR(hdmi->hdmi)) {
		ret = PTR_ERR(hdmi->hdmi);
		goto err_bind;
	}

	return 0;

err_bind:
	drm_encoder_cleanup(encoder);

	return ret;

When dw_hdmi_bind() fails (for example with -EPROBE_DEFER when the DDC I2C
adapter is not ready), component_bind() calls devres_release_group(), which
frees hdmi and disables the devm-managed clocks and regulators while leaving
dev->driver_data as a non-NULL dangling pointer.

Similarly, dw_hdmi_rockchip_unbind() does not clear dev->driver_data before
component_unbind() calls devres_release_group():

static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
				    void *data)
{
	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);

	dw_hdmi_unbind(hdmi->hdmi);
	drm_encoder_cleanup(&hdmi->encoder.encoder);
}

If a system suspend/resume occurs after a failed bind or after component
unbind, wouldn't dev_get_drvdata(dev) in dw_hdmi_rockchip_resume_early()
return a non-NULL dangling pointer and pass a freed or ERR_PTR hdmi->hdmi to
dw_hdmi_resume() while the HDMI clocks and regulators are disabled?

>  
>  	return 0;
>  }
>  
>  static const struct dev_pm_ops dw_hdmi_rockchip_pm = {
> -	SET_SYSTEM_SLEEP_PM_OPS(NULL, dw_hdmi_rockchip_resume)
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(NULL, dw_hdmi_rockchip_resume_early)
>  };
>  
>  struct platform_driver dw_hdmi_rockchip_pltfm_driver = {

നേരിട്ട്

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-349-sashal@kernel.org?part=1