Re: [PATCH v9 04/22] drm/bridge: synopsys: dw-dp: Cancel pending HPD work

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch uses `devm_work_autocancel()` to stop the HPD worker, but relies on devres cleanup which executes during device removal rather than component unbind, leaving a TOCTOU race window where the worker can access freed memory.
- [High] Bridge registration occurs before probe completion, leading to potential Use-After-Free if a subsequent probe step fails.
- [High] Sleeping delay and polling functions are called from atomic context in `dw_dp_bridge_atomic_disable()`.
- [Medium] Unbounded loop in `dw_dp_link_clock_recovery` can cause a kernel thread hang.
--

--- Patch [4]: [PATCH v9 04/22] drm/bridge: synopsys: dw-dp: Cancel pending HPD work ---
commit c38b05d3219e4122c1777f880ab0fd447f213375
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Cancel pending HPD work
    
There is a race condition during device removal: If the HPD interrupt
started an HPD worker exactly when the device is removed, the worker
thread might access invalid resources. Avoid this by stopping any
pending work immediately after disabling the interrupt.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index e332d66b80571..278953a8549a2 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -2120,6 +2120,10 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  	if (dp->irq < 0)
>  		return ERR_PTR(dp->irq);
>  

[Severity: High]
This is a pre-existing issue, but can this error path lead to a
use-after-free?

Just before this block in dw_dp_probe(), the bridge is registered globally:

    ret = devm_drm_bridge_add(dev, bridge);
    if (ret)
            return ERR_PTR(ret);

If a subsequent step like devm_work_autocancel() or
devm_request_threaded_irq() fails, the probe aborts and devres cleans up
the allocated memory. If another driver discovers and references the bridge
between registration and this failure, will it dereference freed memory?

> +	ret = devm_work_autocancel(dev, &dp->hpd_work, dw_dp_hpd_work);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this fix fully resolve
the race condition during component unbind?

The patch uses devm_work_autocancel() to stop the HPD worker during device
removal (devres cleanup). However, dw_dp_unbind() is currently empty and
does not disable the IRQ or cancel the work:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_unbind() {
    /* nothing to do */
}

If the component master unbinds, it initiates the destruction of the DRM
device while the worker might still be scheduled. The worker locklessly reads
dp->bridge.dev:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_hpd_work() {
    ...
    if (!dp->bridge.dev)
            return;
    ...
}

This creates a time-of-check to time-of-use race with drm_bridge_detach(),
which clears the device pointer. Could this lead to a use-after-free when
the worker proceeds to use the freed DRM device in dw_dp_link_retrain()?

>  	ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
>  					IRQF_ONESHOT, dev_name(dev), dp);
>  	if (ret) {


While reviewing this file, I noticed a couple of other pre-existing issues
that might need attention:

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential unbounded loop
in dw_dp_link_clock_recovery():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_link_clock_recovery() {
    ...
    for (;;) {
        ...
        adj_changed = dw_dp_link_get_adjustments(link, status);
        if (!adj_changed)
            tries++;
        else
            tries = 0;

        if (tries == 5)
            break;
    }
}

If a non-compliant or malicious DP sink continually alternates its
adjustment requests, dw_dp_link_get_adjustments() will continuously
return true. Since tries is repeatedly reset to 0, does this prevent
the loop from ever exiting and cause a kernel thread hang?


[Severity: High]
This isn't a bug introduced by this patch, but does the bridge disable
sequence sleep in atomic context?

During a DRM atomic commit, drm_atomic_helper_commit_tail() runs in an
atomic context. It disables bridges, calling dw_dp_bridge_atomic_disable(),
which calls dw_dp_reset() unconditionally:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bridge_atomic_disable() {
    ...
    dw_dp_reset(dp);
    pm_runtime_put_autosuspend(dp->dev);
}

Inside dw_dp_reset(), the code calls sleeping functions:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_reset() {
    ...
    usleep_range(10, 20);
    ...
    regmap_read_poll_timeout(dp->regmap, DW_DP_HPD_STATUS, val,
                             FIELD_GET(HPD_HOT_PLUG, val), 200, 200000);
}

Can this violation of kernel locking rules lead to warnings and potential
deadlocks during an atomic mode-set or display disable sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312@collabora.com?part=4