Re: [PATCH v9 24/61] drm/rockchip: dw_hdmi_qp: Fix NULL deref in PM ops on incomplete bind

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

Pre-existing issues:
- [Critical] Use-After-Free in system-sleep PM callbacks due to dangling drvdata pointer when component bind fails or unbinds.
--

--- Patch [24]: [PATCH v9 24/61] drm/rockchip: dw_hdmi_qp: Fix NULL deref in PM ops on incomplete bind ---
commit bcecf290cbabd0264ffa7bbd8cb566bf124348f0
Author: Cristian Ciocaltea <[email protected]>

drm/rockchip: dw_hdmi_qp: Fix NULL deref in PM ops on incomplete bind

The system-sleep PM callbacks fetch the driver state via
dev_get_drvdata() and dereference it unconditionally. This patch adds
NULL pointer checks for the drvdata pointer in the suspend and resume
callbacks. It also sets the hdmi->hdmi pointer to NULL on bind failure.

Closes: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 9c4cf68d79d50..7c9d26d796642 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -686,9 +686,12 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	platform_set_drvdata(pdev, hdmi);
>  
>  	hdmi->hdmi = dw_hdmi_qp_bind(pdev, encoder, &plat_data);
> -	if (IS_ERR(hdmi->hdmi))
> -		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->hdmi),
> +	if (IS_ERR(hdmi->hdmi)) {
> +		ret = PTR_ERR(hdmi->hdmi);
> +		hdmi->hdmi = NULL;
> +		return dev_err_probe(hdmi->dev, ret,
>  				     "Failed to bind dw-hdmi-qp");
> +	}

[Severity: Critical]
This is a pre-existing issue, but does this error path leave a dangling
pointer in drvdata?

Since the hdmi structure is allocated using drmm_kzalloc(), its lifetime is
tied to the DRM master device. If dw_hdmi_qp_bind() fails, this function
returns an error and the DRM master tears down, which frees the hdmi pointer.

However, since platform_set_drvdata(pdev, hdmi) was already called, this
leaves a dangling pointer in drvdata.

Also, dw_hdmi_qp_rockchip_unbind() completes without clearing the drvdata
pointer, which causes the same dangling pointer when the DRM master is
destroyed.

[ ... ]

> @@ -735,12 +739,15 @@ static int __maybe_unused dw_hdmi_qp_rockchip_resume(struct device *dev)
>  {
>  	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
>  
> -	hdmi->ctrl_ops->io_init(hdmi);
> +	if (hdmi) {
> +		hdmi->ctrl_ops->io_init(hdmi);

[Severity: Critical]
This is a pre-existing issue, but does this lead to a Use-After-Free if
drvdata holds a dangling pointer?

Because the error path in dw_hdmi_qp_rockchip_bind() and the unbind function
do not clear the drvdata pointer, dev_get_drvdata(dev) can return a non-NULL
freed pointer.

The if (hdmi) check succeeds on the invalid pointer, and
hdmi->ctrl_ops->io_init(hdmi) will dereference a function pointer from freed
memory. Could this lead to arbitrary code execution if the system is suspended
or resumed?

> -	dw_hdmi_qp_resume(dev, hdmi->hdmi);
> +		if (hdmi->hdmi)
> +			dw_hdmi_qp_resume(dev, hdmi->hdmi);
>  
> -	if (hdmi->encoder.encoder.dev)
> -		drm_helper_hpd_irq_event(hdmi->encoder.encoder.dev);
> +		if (hdmi->encoder.encoder.dev)
> +			drm_helper_hpd_irq_event(hdmi->encoder.encoder.dev);
> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=24