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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Setting `notifier_call` to NULL during unregistration causes a NULL pointer dereference in concurrent extcon notifier traversals.

Pre-existing issues:
- [High] The extcon notifier callback can queue `extcon_wq` after the work has been cancelled, leading to a Use-After-Free of the `it6505` structure.
--

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

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

The patch attempts to fix use-after-free issues during device removal by
unregistering the extcon notifier, disabling IRQs, and cancelling work items
before the devres cleanup frees the it6505 structure. It also initializes the
extcon workqueue in the probe function.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 8ecb43611dba8..e9c84e4fb3a09 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -2951,13 +2954,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->event_nb.notifier_call) {
> +		devm_extcon_unregister_notifier(it6505->dev, it6505->extcon,
> +						EXTCON_DISP_DP,
>  						&it6505->event_nb);
> -
> -		flush_work(&it6505->extcon_wq);
> +		it6505->event_nb.notifier_call = NULL;

[Severity: High]
Does setting notifier_call to NULL here cause a NULL pointer dereference in
concurrent extcon notifier traversals?

Extcon uses raw notifiers, which do not provide built-in locking or RCU grace
periods during traversal. If an extcon state change triggers extcon_sync()
(which calls raw_notifier_call_chain()) concurrently with this unbind process,
setting it6505->event_nb.notifier_call to NULL while the traversal thread is
examining this notifier block can cause a crash.

If the traversal thread fetches nb->notifier_call inside notifier_call_chain()
immediately after it is set to NULL, it will attempt to execute it.

>  	}
> +	mutex_unlock(&it6505->extcon_lock);
> +
> +	flush_work(&it6505->extcon_wq);
>  }

[ ... ]

> @@ -3647,6 +3654,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);

[Severity: High]
This is a pre-existing issue, but does this fully prevent a Use-After-Free
if the extcon notifier callback is running concurrently?

In it6505_remove_notifier_module(), devm_extcon_unregister_notifier() removes
the raw notifier but does not wait for concurrently executing callbacks to finish.
If the it6505_extcon_notifier() callback is executing or preempted on another
CPU, it can call schedule_work(&it6505->extcon_wq) after cancel_work_sync()
returns.

When the device unbinds, devres frees the it6505 structure, which can cause
the scheduled workqueue to dereference freed memory.

>  	it6505_poweroff(it6505);
>  	it6505_remove_edid(it6505);
>  }

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