Re: [PATCH v10 02/21] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

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

New issues:
- [High] Race condition leading to uninitialized mutex usage in bridge attach path.

Pre-existing issues:
- [High] Use-After-Free of bridge resources on component removal while DRM device is open.
--

--- Patch [2]: [PATCH v10 02/21] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback ---
commit add1fb610af93986b5b39118795cddc285343d20
Author: Sebastian Reichel <[email protected]>

drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

Currently the driver requests resources in the bind function which are
freed on driver removal instead of unbind. This introduces a probe helper
to fix the discrepancy and keeps interrupts disabled while detached to
prevent spurious worker wakeups.

Fixes: 86eecc3a9c2e ("drm/bridge: synopsys: Add DW DPTX Controller support library")
Reported-by: Sashiko <[email protected]>

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 112a499113095..f8ca464d43717 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1826,10 +1826,19 @@ static int dw_dp_bridge_attach(struct drm_bridge *bridge,
>  	dp->aux.name = dev_name(dev);
>  	dp->aux.transfer = dw_dp_aux_transfer;
>  
> +	enable_irq(dp->irq);
> +
>  	ret = drm_dp_aux_register(&dp->aux);

[Severity: High]
Can this race with the interrupt handler?

If the display is already plugged in and the interrupt line is asserted,
dw_dp_irq() might fire immediately after enable_irq() is called. This
schedules the dp->hpd_work worker.

The worker function dw_dp_hpd_work() can then execute before
drm_dp_aux_register() completes. It calls dw_dp_link_retrain(), which in
turn calls drm_dp_dpcd_read_link_status(). That function acquires the
aux->hw_mutex lock.

Since hw_mutex is exclusively initialized inside drm_dp_aux_init(), which
is called by drm_dp_aux_register(), could the worker attempt to lock an
uninitialized mutex here?

> -	if (ret)
> +	if (ret) {
>  		dev_err(dev, "Aux register failed: %d\n", ret);
> +		goto err_disable_irq;
> +	}
> +
> +	return 0;
>  
> +err_disable_irq:
> +	disable_irq(dp->irq);
> +	cancel_work_sync(&dp->hpd_work);
>  	return ret;
>  }
>  
> @@ -1838,6 +1847,8 @@ static void dw_dp_bridge_detach(struct drm_bridge *bridge)
>  	struct dw_dp *dp = bridge_to_dp(bridge);
>  
>  	drm_dp_aux_unregister(&dp->aux);
> +	disable_irq(dp->irq);
> +	cancel_work_sync(&dp->hpd_work);
>  }

[Severity: High]
This is a pre-existing issue, but does this lead to a use-after-free
on component removal?

The dw_dp structure is allocated using devm_drm_bridge_alloc in
dw_dp_probe(), tying its lifetime to the component platform device.
If the component is unbound, its devm resources are immediately freed
when the remove function finishes.

However, if a userspace application holds the DRM device file
descriptor open, the overarching DRM device is not torn down
immediately. When userspace eventually closes the file descriptor,
drm_encoder_cleanup() traverses the bridge chain and calls
dw_dp_bridge_detach().

Does this cause dw_dp_bridge_detach() to access the already freed dp
pointer when it tries to unregister the aux channel, disable the irq,
and cancel the work?

[ ... ]

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