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

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

New issues:
- [High] Moving `drm_dp_aux_unregister()` to `dw_dp_bridge_detach()` decouples the AUX device lifecycle from the component device's `devm_` resources, causing a Use-After-Free vulnerability on unbind or error paths.

Pre-existing issues:
- [High] The AUX transfer function does not reinitialize the completion object before waiting, leading to state corruption on timeout.
--

--- Patch [1]: [PATCH v8 01/21] drm/bridge: synopsys: dw-dp: Register DP AUX on bridge attach ---
commit 356b23a8a66946521d5979525583ffc3eb67acf6
Author: Sebastian Reichel <[email protected]>

drm/bridge: synopsys: dw-dp: Register DP AUX on bridge attach

This commit moves the DP AUX device registration from the bind/unbind
callbacks into the bridge attach/detach callbacks. This is intended to
ensure the AUX device is only accessible when the DRM device is ready
and the bridge is powered.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 3445c82e6f50e..112a499113095 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)
> +{

[ ... ]

> +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]
Does moving the unregistration here decouple the AUX device lifecycle
from the component device's devres resources?

If a user unbinds the component device via sysfs while the DRM master
device is held open by userspace, the component unbinds and its devres
group frees dp and dp->regmap.

However, the DRM bridge remains attached until the DRM device is fully
closed, delaying dw_dp_bridge_detach(). The AUX char device
/dev/drm_dp_auxN would remain registered and accessible, backed by freed
memory. 

A subsequent AUX transfer could dereference the freed dp->regmap, and
when the DRM device is finally closed, drm_encoder_cleanup() would call
dw_dp_bridge_detach(), which calls drm_dp_aux_unregister() on the freed
dp memory.

[ ... ]

> @@ -2065,37 +2084,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);
>  	}

[Severity: High]
Can returning directly on these error paths without unregistering the AUX
device or detaching the bridge cause a use-after-free regression?

The dw_dp_bind() function allocates dp and dp->regmap using devm_ APIs.
Since the synchronous drm_dp_aux_unregister() was removed from the error
paths, the AUX device remains registered. When bind fails, the devres
group will free dp, leaving a dangling pointer.

[ ... ]

>  void dw_dp_unbind(struct dw_dp *dp)
>  {
> -	drm_dp_aux_unregister(&dp->aux);
> +	/* nothing to do */
>  }

[Severity: High]
By removing the synchronous AUX unregistration here, could the char device
outlive the component and lead to the use-after-free described above?

[Severity: High]
This is a pre-existing issue, but does dw_dp_aux_transfer() fail to
reinitialize the completion object before waiting?

Looking at dw_dp_aux_transfer() in drivers/gpu/drm/bridge/synopsys/dw-dp.c:

	regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

	status = wait_for_completion_timeout(&dp->complete, timeout);

If a transfer times out, it returns without decrementing the completion's
internal counter. When the hardware eventually completes the delayed
transfer, the IRQ handler calls complete(&dp->complete).

On the next AUX transfer, wait_for_completion_timeout() will immediately
return success by consuming the stray completion instead of waiting for
the new transfer. The driver would then read DW_DP_AUX_STATUS containing
in-progress or stale data.

Should reinit_completion(&dp->complete) be called before initiating the
hardware transfer?

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