Re: [PATCH v9 01/22] 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 bridge `detach` callback defers it to the DRM device lifecycle, causing a Use-After-Free when the physical 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 resource 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 AUX on bridge attach ---
commit cf33bafacfb642286c8ee7eb20855f566f32f630
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
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/bridge/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_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() incorrectly
handle completions?

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

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

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

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

> +
> +	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]
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 triggers
devres to run for the unbound physical device, freeing dp->regmap and disabling
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 transfer
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, struct drm_encoder *encoder,
[ ... ]
>  	ret = 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 rebinding?

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 prior
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);
>  	}
>  
>  	return dp;
> -
> -unregister_aux:
> -	drm_dp_aux_unregister(&dp->aux);
> -	return ERR_PTR(ret);
>  }
>  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 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 right
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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312@collabora.com?part=1