Re: [PATCH v11 01/21] drm/bridge: synopsys: dw-dp: Register DP AUX on bridge attach

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving `drm_dp_aux_unregister()` to the `.detach` bridge hook introduces a use-after-free vulnerability during driver unbind.

Pre-existing issues:
- [High] Using `devm_` APIs inside the component `bind` callback causes resource leaks and permanent probe failures if the aggregate device defers probing.
- [High] Missing `reinit_completion()` in `dw_dp_aux_transfer()` leads to permanent completion desynchronization after an initial timeout.
- [High] Missing cancellation of `dp->hpd_work` during unbind/remove leads to a Use-After-Free if a hotplug interrupt arrives just before the module is unloaded.
--

--- Patch [1]: [PATCH v11 01/21] drm/bridge: synopsys: dw-dp: Register DP AUX on bridge attach ---
commit c6879b7a6958b5129284c9fb809eb29af0d93f7c
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Register DP AUX on bridge attach
    
This commit delays the registration of the DP AUX device until the bridge
is attached, and unregisters it in the detach hook. This helps ensure that
it is only accessed when the DRM device is ready.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 8b8529e1723c8..60feb3d1e14bb 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1813,7 +1813,36 @@ static struct drm_bridge_state *dw_dp_bridge_atomic_duplicate_state(struct drm_b
>  	return &state->base;
>  }
>  
> +static int dw_dp_bridge_attach(struct drm_bridge *bridge,
> +			       struct drm_encoder *encoder,
> +			       enum drm_bridge_attach_flags flags)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +	struct device *dev = dp->dev;
> +	int ret;
> +
> +	dp->aux.dev = dev;
> +	dp->aux.drm_dev = encoder->dev;
> +	dp->aux.name = dev_name(dev);
> +	dp->aux.transfer = dw_dp_aux_transfer;

[Severity: High]
This is a pre-existing issue, but does dw_dp_aux_transfer() permanently
desynchronize after a single timeout?

If an AUX transaction times out, wait_for_completion_timeout() returns
and dp->complete.done remains 0. When the late hardware interrupt
eventually arrives, dw_dp_irq() calls complete(), setting done to 1.

Since dw_dp_aux_transfer() lacks a preceding call to reinit_completion(),
the very next AUX transfer will immediately succeed the wait without actually
waiting for the hardware, leading to premature reads of DW_DP_AUX_STATUS.

> +
> +	ret = drm_dp_aux_register(&dp->aux);
> +	if (ret)
> +		dev_err(dev, "Aux register failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void dw_dp_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +
> +	drm_dp_aux_unregister(&dp->aux);

[Severity: High]
Will moving drm_dp_aux_unregister() to the bridge detach hook cause a
use-after-free if userspace keeps the DRM device open?

If a process holds an open file descriptor for the DRM device while the
driver is unbound, the DRM device's refcount prevents the bridge's detach
hook from running immediately.

The component's memory (like the dp structure) will be freed by the driver
core during unbind, but the AUX char device remains registered. If userspace
then reads or writes to the AUX device, dw_dp_aux_transfer() will
dereference the freed memory.

> +}
> +

[ ... ]

> @@ -2064,37 +2083,33 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
>  	ret = phy_init(dp->phy);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "phy init failed\n");
> -		goto unregister_aux;
> +		return ERR_PTR(ret);
>  	}
>  
>  	ret = devm_add_action_or_reset(dev, dw_dp_phy_exit, dp);
>  	if (ret)
> -		goto unregister_aux;
> +		return ERR_PTR(ret);
>  
>  	dp->irq = platform_get_irq(pdev, 0);

[Severity: High]
This isn't a bug introduced by this patch, but can using devm_ APIs inside
the component bind callback cause resource leaks?

If the aggregate DRM device defers probing, the component framework undoes
the bind, but it does not release the devm_ resources because the component
device itself remains bound to its platform driver.

When the aggregate device retries probing, dw_dp_bind() allocates them
again. The calls to devm_request_threaded_irq() or
devm_platform_ioremap_resource() will fail with -EBUSY, permanently
preventing the pipeline from probing.

>  	if (dp->irq < 0) {
>  		ret = dp->irq;
> -		goto unregister_aux;
> +		return ERR_PTR(ret);
>  	}
>  
>  	ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,

[ ... ]

> @@ -2101,8 +2112,8 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
>  
>  void dw_dp_unbind(struct dw_dp *dp)
>  {
> -	drm_dp_aux_unregister(&dp->aux);
> +	/* nothing to do */
>  }

[Severity: High]
This is a pre-existing issue, but is it possible for an HPD interrupt to
race with module unload?

If an HPD interrupt triggers and schedules dp->hpd_work just before the
driver is unbound, the global workqueue may execute dw_dp_hpd_work() after
the devm_ cleanup path frees the dp structure.

Does dw_dp_unbind() need a call to cancel_work_sync(&dp->hpd_work) to
prevent a use-after-free?

>  EXPORT_SYMBOL_GPL(dw_dp_unbind);

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