Re: [PATCH v8 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] The DRM bridge is registered to the subsystem before its internal resources (IRQ and workqueue) are fully initialized, creating a race condition that leads to a Use-After-Free.
- [High] The completion structure `dp->complete` is not reinitialized before an AUX transfer, causing subsequent transfers to prematurely return if a previous transfer timed out.
- [High] The shared `dp->link` state is modified without locking during connector detection, racing with link training in the atomic modeset path.
--

--- Patch [5]: [PATCH v8 05/21] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal ---
commit a0602d7d2fea7b25f801546caecd555dc659c65d
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 a8ae13cf91d60..f373643a9cc10 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2027,9 +2027,12 @@ 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_completion(&dp->complete);

[Severity: High]
This is a pre-existing issue, but since we are looking at the initialization
of dp->complete, I noticed that dw_dp_aux_transfer() waits for this
completion without reinitializing it:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer() {
	...
	regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

	status = wait_for_completion_timeout(&dp->complete, timeout);
	if (!status) {
		dev_err(dp->dev, "timeout waiting for AUX reply\n");
	...
}

If a transfer times out, wait_for_completion_timeout() returns -ETIMEDOUT
and leaves the completion state unchanged.

If the hardware later generates a delayed interrupt and dw_dp_irq() calls
complete(&dp->complete), wouldn't the next invocation of
dw_dp_aux_transfer() immediately return success by consuming the stale
completion before the new transaction finishes?

>  
> +	ret = devm_mutex_init(dev, &dp->irq_lock);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
dw_dp_probe(), could the probe error handling cause a Use-After-Free?

The bridge is registered globally before the IRQ is requested:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_probe() {
	...
	ret = devm_drm_bridge_add(dev, bridge);
	if (ret)
		return ERR_PTR(ret);

	dp->irq = platform_get_irq(pdev, 0);
	if (dp->irq < 0)
		return ERR_PTR(dp->irq);
	...
}

If devm_request_threaded_irq() or another subsequent step fails, devres will
free the struct dw_dp memory.

If a KMS driver probing concurrently has already found and attached to the
bridge via of_drm_find_bridge(), wouldn't it hold a dangling pointer to the
freed bridge?

[Severity: High]
This is another pre-existing issue, but I wanted to ask about the locking
around the shared dp->link state.

When userspace triggers connector detection, dw_dp_link_parse()
zero-initializes dp->link fields:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_link_parse() {
	...
	dw_dp_link_reset(link);

	ret = drm_dp_read_dpcd_caps(&dp->aux, link->dpcd);
	...
}

Concurrently, a non-blocking atomic modeset commit executing in a workqueue
might call dw_dp_link_configure(), which reads those same fields:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_link_configure() {
	...
	ret = dw_dp_phy_configure(dp, link->rate, link->lanes, link->caps.ssc);
	...
}

If the read in dw_dp_link_configure() races with the reset in
dw_dp_link_parse(), could the driver program link->rate = 0 and
link->lanes = 0 into the PHY registers?

>  	res = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(res))
>  		return ERR_CAST(res);

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