Re: [PATCH v10 05/21] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cancellation of asynchronous hpd_work in devres teardown leads to Use-After-Free.
- [High] TOCTOU race in dw_dp_link_retrain() can cause DP link training while the PHY is powered off.
- [High] Missing reinit_completion() in dw_dp_aux_transfer() breaks subsequent AUX transfers after a timeout.
--

--- Patch [5]: [PATCH v10 05/21] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal ---
commit 58a91370c6467a81d51233b812a70d21f2296081
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal

The driver is currently missing to fully clean up after itself. Ensure
that the mutex is cleaned up.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index fd7663013faaa..535af11e617c4 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2045,10 +2045,13 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  	dp->pixel_mode = plat_data->pixel_mode;
>  	dp->plat_data.max_link_rate = plat_data->max_link_rate;
>  
> -	mutex_init(&dp->irq_lock);
>  	INIT_WORK(&dp->hpd_work, dw_dp_hpd_work);

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing cleanups
on module removal here, does the asynchronous hpd_work also need to be
canceled during devres teardown?

If the driver is unbound, devm_request_threaded_irq will free the IRQ. If a
hotplug event occurred right before this, hpd_work could be scheduled. Since
there is no devm action to cancel the work, the work handler dw_dp_hpd_work()
could execute after devres has freed the struct dw_dp memory and destroyed
the irq_lock mutex, leading to a use-after-free when it calls
mutex_lock(&dp->irq_lock).

>  	init_completion(&dp->complete);

[Severity: High]
This is also a pre-existing issue, but I noticed dw_dp_aux_transfer() uses
this completion without reinitializing it.

If an AUX transfer times out:

dw_dp_aux_transfer() {
    ...
    regmap_write(dp->regmap, DW_DP_AUX_CMD, value);
    status = wait_for_completion_timeout(&dp->complete, timeout);
    ...
}

wait_for_completion_timeout() returns 0 without consuming the completion's
internal done count. If the hardware interrupt fires shortly after, the
handler calls complete(&dp->complete), leaving the completion in a signaled
state.

Since dw_dp_aux_transfer() does not call reinit_completion() before starting
a new transfer, will the next transfer immediately fall through
wait_for_completion_timeout(), reading stale hardware status?

>  
> +	ret = devm_mutex_init(dev, &dp->irq_lock);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	res = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(res))
>  		return ERR_CAST(res);

[Severity: High]
This is another pre-existing issue not introduced by this patch, but looking
at the hpd_work handler, is there a TOCTOU race in dw_dp_link_retrain() that
can cause DP link training while the PHY is powered off?

dw_dp_link_retrain() checks the link state before acquiring the mutex:

dw_dp_link_retrain() {
    if (!dw_dp_needs_link_retrain(dp))
        return 0;
    ...
    drm_modeset_acquire_init(&ctx, 0);
    for (;;) {
        ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
        ...
    }
    if (!ret)
        ret = dw_dp_link_train(dp);
    ...
}

dw_dp_link_retrain() is called asynchronously from hpd_work. Concurrently,
userspace could issue an atomic commit to disable the display, which acquires
connection_mutex and disables the link (powering off the PHY via
dw_dp_link_disable). 

When the atomic commit releases the mutex, dw_dp_link_retrain() acquires it
and unconditionally calls dw_dp_link_train(). Could this result in hardware
lockups from attempting to read or write registers of a powered-off PHY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v10-0-d84a26c303d6@collabora.com?part=5