Re: [PATCH v6 03/13] drm/bridge: it6505: unregister DP AUX adapter on bridge detach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing `drm_dp_aux_unregister()` from the I2C `remove` function causes a use-after-free and leaves a zombie DP AUX adapter if the I2C driver is unbound while the DRM device remains attached.
--

--- Patch [3]: [PATCH v6 03/13] drm/bridge: it6505: unregister DP AUX adapter on bridge detach ---
commit d6c69e9e1c11461da60c039acb301f1120930d47
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: unregister DP AUX adapter on bridge detach

This commit moves the DP AUX adapter unregistration from the I2C remove
routine to the DRM bridge detach and attach error paths, to avoid
registering the adapter multiple times during unbind/rebind cycles.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 7c61b02cc7e60..c10cc6a786b5d 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
> @@ -3106,6 +3107,7 @@ static void it6505_bridge_detach(struct drm_bridge *bridge)
>  
>  	flush_work(&it6505->link_works);
>  	it6505_remove_notifier_module(it6505);
> +	drm_dp_aux_unregister(&it6505->aux);
>  }
>  
[ ... ]
> @@ -3654,7 +3656,6 @@ static void it6505_i2c_remove(struct i2c_client *client)
>  	struct it6505 *it6505 = i2c_get_clientdata(client);
>  
>  	drm_bridge_remove(&it6505->bridge);
> -	drm_dp_aux_unregister(&it6505->aux);

[Severity: High]
Does removing drm_dp_aux_unregister() here cause a use-after-free if the I2C
driver is unbound while the DRM device is still attached?

If the I2C driver is unbound (e.g., via sysfs) while the DRM display
controller is still bound, it6505_i2c_remove() is called. The
drm_bridge_remove() call only moves the bridge to a lingering list and
does not immediately call it6505_bridge_detach().

Because drm_dp_aux_unregister() was removed from this path, the AUX adapter
remains registered. When the remove function returns, devres frees the
it6505 structure. 

Could a local user trigger a use-after-free by accessing the leaked
/dev/drm_dp_auxN character device or I2C sysfs nodes which now point to
the freed structure?

Since i2c_del_adapter() and drm_dp_aux_unregister_devnode() both contain
safety guards against unregistered adapters, is it safer to leave
drm_dp_aux_unregister() in the remove path as well to ensure it is always
unregistered before devres frees the memory?

>  	it6505_debugfs_remove(it6505);
>  	it6505_remove_notifier_module(it6505);
>  	disable_irq(it6505->irq);

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