diff mbox series

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

Message ID 20260723-dw-hdmi-qp-scramb-v9-24-4fb12ea22ac9@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea July 23, 2026, 1:35 a.m. UTC 
The system-sleep PM callbacks fetch the driver state via
dev_get_drvdata() and dereference it unconditionally.  However, they can
run while the device is not fully bound.  If the system suspends before
the component framework invokes the bind callback (e.g. during a
deferred probe waiting for the VOP), dev_get_drvdata() returns NULL.
Moreover, if dw_hdmi_qp_bind() fails, drvdata is already set but
hdmi->hdmi is left holding an error pointer.

In either case dw_hdmi_qp_rockchip_resume() dereferences an invalid
pointer, causing a kernel panic; the suspend path is equally affected.

Guard both callbacks against NULL checks, and reset hdmi->hdmi to NULL
when dw_hdmi_qp_bind() fails so the guards are effective.
hdmi->ctrl_ops is assigned before platform_set_drvdata(), so it is safe
to use whenever drvdata is non-NULL.

Fixes: 128a9bf8ace2 ("drm/rockchip: Add basic RK3588 HDMI output support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260702151550.8E14D1F000E9@smtp.kernel.org/
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

sashiko-bot@kernel.org July 23, 2026, 1:48 a.m. UTC | #1 
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 <cristian.ciocaltea@collabora.com>

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/20260702151550.8E14D1F000E9@smtp.kernel.org/

> 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;
>  }
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 9c4cf68d79d5..7c9d26d79664 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");
+	}
 
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector))
@@ -726,7 +729,8 @@  static int __maybe_unused dw_hdmi_qp_rockchip_suspend(struct device *dev)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
 
-	dw_hdmi_qp_suspend(dev, hdmi->hdmi);
+	if (hdmi && hdmi->hdmi)
+		dw_hdmi_qp_suspend(dev, hdmi->hdmi);
 
 	return 0;
 }
@@ -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);
 
-	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;
 }