Re: [PATCH v2 08/78] drm/bridge: adv7511: Switch to atomic_create_state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The HPD workqueue (adv7511->hpd_work) is not cancelled when the driver is unbound or fails to probe after the IRQ is registered, leading to a Use-After-Free.
--

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

drm/bridge: adv7511: Switch to atomic_create_state

This commit deprecates the drm_bridge_funcs.atomic_reset callback and its
helpers. It switches the adv7511 bridge driver to use atomic_create_state
and drm_atomic_helper_bridge_create_state instead.

> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 02f8f7e78a163..db99e252aeebc 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1004,7 +1004,7 @@ static const struct drm_bridge_funcs adv7511_bridge_funcs = {
>  	.atomic_disable = adv7511_bridge_atomic_disable,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> -	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_create_state = drm_atomic_helper_bridge_create_state,
>  
>  	.hdmi_tmds_char_rate_valid = adv7511_bridge_hdmi_tmds_char_rate_valid,

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing
issue in this driver's error handling and removal paths regarding the HPD
workqueue.

In adv7511_probe(), an IRQ handler is registered via
devm_request_threaded_irq() which can schedule adv7511->hpd_work. If the
probe fails after this point, the error path does not cancel the work
before returning:

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:adv7511_probe() {
...
err_unregister_audio:
	drm_bridge_remove(&adv7511->bridge);
	i2c_unregister_device(adv7511->i2c_cec);
...
}

Similarly, adv7511_remove() does not cancel the work either:

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:adv7511_remove() {
	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);

	of_node_put(adv7511->host_node);

	adv7511_uninit_regulators(adv7511);

	drm_bridge_remove(&adv7511->bridge);
...
}

Because devres eventually frees the IRQ but free_irq() does not cancel
already scheduled work, could this lead to a use-after-free?

If the work remains pending after the final reference to the adv7511
structure is dropped, any executing adv7511_hpd_work() would dereference
the freed memory.

Should cancel_work_sync(&adv7511->hpd_work) be called in these paths to
prevent the work from executing after the structure is freed?

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