Re: [PATCH v2 02/78] drm/bridge: cdns-mhdp8546: Return an error pointer on allocation failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Asynchronous firmware loading callback can execute after the device is unbound, causing a Use-After-Free on the devres-allocated `mhdp` structure.
- [High] Unbalanced teardown of `hpd_work` and `devm_request_threaded_irq` allows a scheduled work to execute after `mhdp` is freed.
- [High] `cdns_mhdp_bridge_atomic_duplicate_state` fails to copy `current_mode`, breaking link retraining and mishandling state lifecycles.
--

commit e5ec8b954cd81adb602c1048268bad60ac236f06
Author: Maxime Ripard <mripard@kernel.org>

    drm/bridge: cdns-mhdp8546: Return an error pointer on allocation failure
    
    The drm_bridge_funcs.atomic_reset documentation states that the hook
    must return either a valid drm_bridge_state object or an ERR_PTR().

> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 36c07b71fe04b..46779b49545bd 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -1927,7 +1927,7 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
>  
>  	cdns_mhdp_state = kzalloc_obj(*cdns_mhdp_state);
>  	if (!cdns_mhdp_state)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	__drm_atomic_helper_bridge_reset(bridge, &cdns_mhdp_state->base);
>  

[Severity: High]
This isn't a bug introduced by this patch, but could this asynchronous firmware
callback cause a use-after-free if the device is unbound?

If the driver is unbound and cdns_mhdp_remove() executes, it only waits up to
100ms for the firmware load to finish:

cdns_mhdp_remove() {
    ...
	ret = wait_event_timeout(mhdp->fw_load_wq,
				 mhdp->hw_state == MHDP_HW_READY,
				 timeout);
    ...
}

If this times out, remove completes and devres frees the mhdp structure. When
the asynchronous firmware subsystem later finishes, it invokes
cdns_mhdp_fw_cb() which dereferences the freed mhdp pointer:

cdns_mhdp_fw_cb() {
	struct cdns_mhdp_device *mhdp = context;
    ...
	dev_dbg(mhdp->dev, "firmware callback\n");
    ...
}

[Severity: High]
This is a pre-existing issue, but does this teardown sequence leave a scheduled
work vulnerable to a use-after-free?

In cdns_mhdp_remove(), the driver calls flush_work() without explicitly
disabling the hardware interrupt:

cdns_mhdp_remove() {
    ...
	flush_work(&mhdp->hpd_work);
    ...
}

Because devm_request_threaded_irq() was used, the IRQ handler remains active
until the devres teardown phase, which occurs after remove completes. If an
interrupt fires between flush_work() and the devres IRQ teardown,
cdns_mhdp_irq_handler() will execute and schedule the work again:

cdns_mhdp_irq_handler() {
    ...
	if (bridge_attached && (sw_ev0 & CDNS_DPTX_HPD)) {
		schedule_work(&mhdp->hpd_work);
	}
    ...
}

Devres will subsequently free the IRQ and the mhdp structure, but the work
remains queued and will eventually run cdns_mhdp_hpd_work(), dereferencing
freed memory.

[Severity: High]
This isn't a bug introduced by this patch, but I noticed
cdns_mhdp_bridge_atomic_duplicate_state() doesn't seem to copy current_mode.
Will this break link retraining?

When a new state is allocated, the subclass-specific current_mode pointer is
left NULL:

cdns_mhdp_bridge_atomic_duplicate_state() {
	struct cdns_mhdp_bridge_state *state;

	state = kzalloc_obj(*state);
	if (!state)
		return NULL;

	__drm_atomic_helper_bridge_duplicate_state(bridge, &state->base);

	return &state->base;
}

During a non-modeset atomic commit, atomic_enable is not called, so
current_mode remains NULL in the new state. The old state is destroyed,
permanently freeing the previous current_mode. If a hotplug event subsequently
triggers a link retrain, it fetches the current state, finds current_mode is
NULL, and fails with -EINVAL, silently breaking display output:

cdns_mhdp_update_link_status() {
    ...
		current_mode = cdns_bridge_state->current_mode;
		if (!current_mode) {
			ret = -EINVAL;
			goto out;
		}
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-drm-no-more-bridge-reset-v2-0-0a91018bf886@kernel.org?part=2