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

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 bridge `detach` callback d=
efers it to the DRM device lifecycle, causing a Use-After-Free when the phy=
sical device is unbound.

Pre-existing issues:
- [High] Missing `reinit_completion()` before starting an AUX transfer can =
cause subsequent transfers to falsely complete after a timeout.
- [High] Using `devm_*` allocations in the component bind callback causes r=
esource leaks and prevents successful rebinding.
- [High] The driver schedules `hpd_work` but never cancels it during unbind=
 or device removal, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v9 01/22] drm/bridge: synopsys: dw-dp: Register DP AU=
X on bridge attach ---
commit cf33bafacfb642286c8ee7eb20855f566f32f630
Author: Sebastian Reichel <[email protected]>
Subject: drm/bridge: synopsys: dw-dp: Register DP AUX on bridge attach

This commit moves the registration and unregistration of the DP AUX device
from the bind/unbind phases to the bridge attach/detach phases. This ensures
that the AUX device is only accessed when the DRM device is ready.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/br=
idge/synopsys/dw-dp.c
> --- 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_atomi=
c_duplicate_state(struct drm_b
>  	return &state->base;
>  }
> =20
> +static int dw_dp_bridge_attach(struct drm_bridge *bridge,
> +			       struct drm_encoder *encoder,
> +			       enum drm_bridge_attach_flags flags)
> +{
> +	struct dw_dp *dp =3D bridge_to_dp(bridge);
> +	struct device *dev =3D dp->dev;
> +	int ret;
> +
> +	dp->aux.dev =3D dev;
> +	dp->aux.drm_dev =3D encoder->dev;
> +	dp->aux.name =3D dev_name(dev);
> +	dp->aux.transfer =3D dw_dp_aux_transfer;

[Severity: High]
This is a pre-existing issue, but does dw_dp_aux_transfer() incorrectly
handle completions?

Looking at dw_dp_aux_transfer(), the code waits for a completion without
clearing any previously pending completion state:

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

If a previous hardware timeout occurred (returning 0), and the hardware lat=
er
finishes the transfer, the interrupt handler will call complete(&dp->comple=
te).

Because reinit_completion() is not called before the next transfer, the
subsequent wait_for_completion_timeout() could instantly consume the leftov=
er
completion and return true without waiting, reading stale or invalid status
from the hardware.

> +
> +	ret =3D 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 =3D bridge_to_dp(bridge);
> +
> +	drm_dp_aux_unregister(&dp->aux);
> +}

[Severity: High]
Could deferring drm_dp_aux_unregister() to the DRM device lifecycle lead to=
 a
use-after-free when the physical device is unbound?

If a userspace process keeps the DRM device FD open, the DRM device remains
active even after the DW-DP physical device is unbound via sysfs. This trig=
gers
devres to run for the unbound physical device, freeing dp->regmap and disab=
ling
clocks.

Because drm_dp_aux_unregister() was moved to dw_dp_bridge_detach(), the AUX
device remains registered. If a userspace process initiates an I2C/AUX tran=
sfer
on the still-registered device node, it will call dw_dp_aux_transfer() which
dereferences the freed dp->regmap.

[ ... ]
> @@ -2065,37 +2084,33 @@ struct dw_dp *dw_dp_bind(struct device *dev, stru=
ct drm_encoder *encoder,
[ ... ]
>  	ret =3D devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
>  					IRQF_ONESHOT, dev_name(dev), dp);

[Severity: High]
This is a pre-existing issue, but does using devm_request_threaded_irq() in=
 the
component bind callback cause resource leaks and prevent successful rebindi=
ng?

Component bind callbacks run when the master DRM device is bound. If the DRM
master is unbound, dw_dp_unbind() is called, but devres does not run because
the component platform device itself wasn't removed.

When the DRM master is bound again, dw_dp_bind() will run and attempt to
request the same exclusive IRQ, which may fail with -EBUSY and leak the pri=
or
allocations, preventing the display pipeline from functioning.

>  	if (ret) {
>  		dev_err_probe(dev, ret, "failed to request irq\n");
> -		goto unregister_aux;
> +		return ERR_PTR(ret);
>  	}
> =20
>  	return dp;
> -
> -unregister_aux:
> -	drm_dp_aux_unregister(&dp->aux);
> -	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(dw_dp_bind);
> =20
>  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 does dw_dp_unbind() need to call
cancel_work_sync(&dp->hpd_work) before the component is unbound?

The interrupt handler schedules dp->hpd_work. If an HPD interrupt fires rig=
ht
before the IRQ is disabled by devres or during component unbind, the work is
queued.

When devres frees dp->regmap, and dw_dp_hpd_work() eventually executes, it
calls dw_dp_hpd_detect() which accesses the freed dp->regmap, resulting in a
use-after-free.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-synopsys-d=
[email protected]?part=3D1