diff mbox series

[v10,29/69] drm/rockchip: dw_hdmi_qp: Fix invalid drvdata access in PM ops

Message ID 20260731-dw-hdmi-qp-scramb-v10-29-294364b2cf15@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea July 31, 2026, 4:19 p.m. UTC 
The system-sleep PM callbacks fetch the driver state via
dev_get_drvdata() and dereference it unconditionally.  However, the
platform device is bound by ->probe(), which merely registers a
component, hence the callbacks may well run when no valid state is
available:

- Before the aggregate driver calls ->bind(), e.g. when the system
  suspends while probing is still deferred waiting for the VOP,
  dev_get_drvdata() returns NULL.

- After ->unbind(), or when ->bind() fails past platform_set_drvdata(),
  drvdata is left pointing to memory obtained via drmm_kzalloc(), which
  is released together with the DRM device, while this platform device
  remains bound and PM-capable.

Publish the driver data only after ->bind() completes successfully, and
clear it in ->unbind().  This ensures that a non-NULL drvdata always
represents live state, allowing each PM callback to rely on a single
NULL check.

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/
Closes: https://lore.kernel.org/all/20260723014811.1F9F01F000E9@smtp.kernel.org/
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

sashiko-bot@kernel.org July 31, 2026, 8:19 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free of `hdmi` struct due to `devm_request_threaded_irq` being used in component bind, leaking the IRQ on unbind.
- [High] The HPD hardirq handler checks the raw SOC status register instead of the interrupt pending status, breaking unplug detection and stealing shared IRQs.
--

--- Patch [29]: [PATCH v10 29/69] drm/rockchip: dw_hdmi_qp: Fix invalid drvdata access in PM ops ---
commit fc12a8c8d5e7177cf76cf4554fb62df0ebde5bbd
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: dw_hdmi_qp: Fix invalid drvdata access in PM ops

The system-sleep PM callbacks fetch the driver state via dev_get_drvdata()
and dereference it unconditionally. This patch ensures non-NULL drvdata
always represents live state, allowing each PM callback to rely on a single
NULL check.

Closes: https://lore.kernel.org/all/20260702151550.8E14D1F000E9@smtp.kernel.org/
Closes: https://lore.kernel.org/all/20260723014811.1F9F01F000E9@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..646a4cb968309 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c

[ ... ]

> @@ -704,6 +704,8 @@ static void dw_hdmi_qp_rockchip_unbind(struct device *dev,
>  {
>  	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
>  
> +	dev_set_drvdata(dev, NULL);
> +
>  	cancel_delayed_work_sync(&hdmi->hpd_work);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to rely on
devm_request_threaded_irq() for the HPD interrupt when lifecycle is managed
via component bind and unbind?

Looking at dw_hdmi_qp_rockchip_bind(), the IRQ is requested with devm on the
component device:

    ret = devm_request_threaded_irq(dev, hdmi->hpd_irq, ...);

Because devm resources are tied to the device's probe and remove lifecycle,
the IRQ is not freed when the component is unbound. When the aggregate DRM
device is torn down, hdmi (allocated via drmm_kzalloc) is freed.

If the HPD interrupt fires afterwards, the still-active IRQ handler will
dereference the freed hdmi pointer.

Also, calling cancel_delayed_work_sync() here doesn't prevent the active IRQ
handler from immediately re-arming the workqueue.

Should this use a non-devm request_threaded_irq() in dw_hdmi_qp_rockchip_bind()
and call free_irq() here in dw_hdmi_qp_rockchip_unbind()?

[Severity: High]
This is also a pre-existing issue, but does the hardirq handler properly handle
interrupts on unplug events?

Looking at dw_hdmi_qp_rk3588_hardirq() and dw_hdmi_qp_rk3576_hardirq(), they
read the raw level register without masking:

    regmap_read(hdmi->regmap, RK3588_GRF_SOC_STATUS1, &intr_stat);
    if (intr_stat) {
        ...

When the HDMI cable is unplugged, the HPD level goes to 0, which triggers the
interrupt. If no other unrelated bits in this 32-bit register are set,
intr_stat evaluates to 0 and the handler returns IRQ_NONE without clearing the
interrupt.

Could this cause the kernel to see an unhandled interrupt and disable it
permanently?

Conversely, if any unrelated bit is 1, it will always return IRQ_WAKE_THREAD,
falsely claiming interrupts for other devices on this shared IRQ line.

Should this check the interrupt pending status instead of the raw SOC status
register?

>  }
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..646a4cb96830 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -683,8 +683,6 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (ret)
 		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder");
 
-	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),
@@ -695,6 +693,8 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
 				     "Failed to init bridge connector\n");
 
+	platform_set_drvdata(pdev, hdmi);
+
 	return 0;
 }
 
@@ -704,6 +704,8 @@  static void dw_hdmi_qp_rockchip_unbind(struct device *dev,
 {
 	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
 
+	dev_set_drvdata(dev, NULL);
+
 	cancel_delayed_work_sync(&hdmi->hpd_work);
 }
 
@@ -726,7 +728,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)
+		dw_hdmi_qp_suspend(dev, hdmi->hdmi);
 
 	return 0;
 }
@@ -735,6 +738,9 @@  static int __maybe_unused dw_hdmi_qp_rockchip_resume(struct device *dev)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
 
+	if (!hdmi)
+		return 0;
+
 	hdmi->ctrl_ops->io_init(hdmi);
 
 	dw_hdmi_qp_resume(dev, hdmi->hdmi);