Re: [PATCH v5 03/11] 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 `it6505_i2c_remove()` introduces a critical Use-After-Free vulnerability if the I2C driver is unbound while the DRM bridge remains attached.
--

--- Patch [3]: [PATCH v5 03/11] drm/bridge: it6505: unregister DP AUX adapter on bridge detach ---
commit b259e088ec0ed97d1f41a3a8463e232914e7c924
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
path to the bridge detach and attach error paths. This is intended to
prevent duplicate registrations when unbinding and rebinding the DRM
device.

> 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

[ ... ]

> @@ -3652,7 +3654,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 introduce a use-after-free
if the I2C driver is unbound while the DRM bridge remains attached?

If the I2C driver is unbound (for example, via sysfs) while the DRM
device is still active, the DP AUX character device (/dev/drm_dp_auxN)
appears to remain registered because drm_bridge_remove() does not invoke
it6505_bridge_detach() when the bridge is removed from the global list.

After it6505_i2c_remove() returns, the driver core's devres cleanup will
execute and free the it6505 structure and its managed resources, such as
the regmap.

If userspace subsequently interacts with the leaked character device,
it can invoke it6505_aux_transfer() which dereferences the freed memory:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_aux_transfer() {
	struct it6505 *it6505 = container_of(aux, struct it6505, aux);
	...
	// accesses freed it6505 and performs I/O on freed it6505->regmap
}

Should drm_dp_aux_unregister() be kept in the I2C remove path as well,
or is there another mechanism guaranteeing the AUX character device is
unregistered before devres cleanup occurs?

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

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