Re: [PATCH v6 01/13] drm/bridge: it6505: quiesce event sources and work on remove()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free in `extcon_wq` due to raw notifier traversal race during driver removal.
- [High] NULL Pointer Dereference in `it6505_extcon_work` when racing with bridge detach.
- [Medium] Data race on `extcon_completion` state between `reinit_completion` and `wait_for_completion_timeout`.
- [Medium] Missing `pm_runtime_disable()` in the I2C driver remove path.
--

--- Patch [1]: [PATCH v6 01/13] drm/bridge: it6505: quiesce event sources and work on remove() ---
commit ec8b6db3474d25da1dc993de7833610db7cf6509
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: quiesce event sources and work on remove()

This patch attempts to prevent use-after-free and NULL pointer dereference
issues during driver removal by unregistering the extcon notifier, disabling
interrupts, and explicitly cancelling work items. It also tracks the extcon
registration state to make unregistration idempotent.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -2951,13 +2956,16 @@ static int it6505_use_notifier_module(struct it6505 *it6505)
>  
>  static void it6505_remove_notifier_module(struct it6505 *it6505)
>  {
> -	if (it6505->extcon) {
> -		devm_extcon_unregister_notifier(it6505->dev,
> -						it6505->extcon,	EXTCON_DISP_DP,
> +	mutex_lock(&it6505->extcon_lock);
> +	if (it6505->extcon && it6505->extcon_registered) {
> +		devm_extcon_unregister_notifier(it6505->dev, it6505->extcon,
> +						EXTCON_DISP_DP,
>  						&it6505->event_nb);
> -
> -		flush_work(&it6505->extcon_wq);
> +		it6505->extcon_registered = false;
>  	}
> +	mutex_unlock(&it6505->extcon_lock);
> +
> +	flush_work(&it6505->extcon_wq);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does unregistering the
extcon notifier fully prevent the work item from being requeued?

Because devm_extcon_unregister_notifier() unlinks the notifier from a raw
notifier chain, it cannot synchronize against a concurrent traversal.

If a traversal thread has already read the node but not yet invoked the
callback, it can execute after this flush_work() completes:

it6505_extcon_notifier()
    schedule_work(&it6505->extcon_wq);

If it6505_i2c_remove() then cancels the work and devres frees the it6505
struct, the preempted traversal thread could resume and requeue the work.
Is there a way this could result in a use-after-free on the freed memory?

[Severity: High]
This is a pre-existing issue, but could this same raw notifier race also
lead to a NULL pointer dereference if it races with bridge detach?

During drm_bridge_detach(), the core sets bridge->dev = NULL. If the
delayed traversal requeues it6505_extcon_work() as described above,
the requeued work will eventually execute:

it6505_extcon_work()
    drm_helper_hpd_irq_event(it6505->bridge.dev);

Will this dereference a NULL pointer since bridge->dev was already cleared?

[ ... ]

> @@ -3615,6 +3623,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
>  	INIT_WORK(&it6505->link_works, it6505_link_training_work);
>  	INIT_WORK(&it6505->hdcp_wait_ksv_list, it6505_hdcp_wait_ksv_list);
>  	INIT_DELAYED_WORK(&it6505->hdcp_work, it6505_hdcp_work);
> +	INIT_WORK(&it6505->extcon_wq, it6505_extcon_work);
>  	init_completion(&it6505->extcon_completion);

[Severity: Medium]
This is a pre-existing issue, but is it safe to use reinit_completion()
in it6505_extcon_work() without synchronization?

The it6505_irq_hpd() handler actively waits on this completion:

it6505_irq_hpd()
    wait_for_completion_timeout(&it6505->extcon_completion, ...)

If an extcon event occurs concurrently, it6505_extcon_work() calls
reinit_completion() directly on it6505->extcon_completion, which resets
the completion state without acquiring the internal wait.lock.

Can this corrupt the completion state or cause the waiter to time out
incorrectly?

>  	memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
>  	it6505->powered = false;
> @@ -3647,6 +3656,12 @@ static void it6505_i2c_remove(struct i2c_client *client)
>  	drm_bridge_remove(&it6505->bridge);
>  	drm_dp_aux_unregister(&it6505->aux);
>  	it6505_debugfs_remove(it6505);
> +	it6505_remove_notifier_module(it6505);
> +	disable_irq(it6505->irq);
> +	cancel_work_sync(&it6505->link_works);
> +	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
> +	cancel_delayed_work_sync(&it6505->hdcp_work);
> +	cancel_work_sync(&it6505->extcon_wq);
>  	it6505_poweroff(it6505);
>  	it6505_remove_edid(it6505);
>  }

[Severity: Medium]
This is a pre-existing issue, but is the remove path missing a call to
pm_runtime_disable()?

The it6505_i2c_probe() function explicitly calls pm_runtime_enable(),
but there doesn't appear to be a corresponding disable in this
it6505_i2c_remove() function.

Does leaving PM runtime enabled after driver unbind leave the device in an
incorrect state or cause reference count leaks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784947240.git.daniel@makrotopia.org?part=1