Re: [PATCH 11/11] drm/bridge: samsung-dsim: don't create a panel_bridge

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

Pre-existing issues:
- [Critical] Manual gpiod_put() on a devres-managed GPIO and failure to clear the dsi->te_gpio pointer leads to UAF and double free.
--

--- Patch [11]: [PATCH 11/11] drm/bridge: samsung-dsim: don't create a panel_bridge ---
commit 3269367ff66fe6dd842314973873ed18b84437ca
Author: Luca Ceresoli <[email protected]>

drm/bridge: samsung-dsim: don't create a panel_bridge

Now a panel_bridge is automatically created for every drm_panel, so this
code would lead to creating a second panel_bridge for the same panel. This
commit simplifies the code by just getting a reference to the already-existing
bridge.

> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e2fc69fc51b60..ca419edb1c1a6 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
[ ... ]
> @@ -1931,21 +1930,9 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>  	if (!remote)
>  		return -ENODEV;
>  
> -	panel = of_drm_find_panel(remote);
> -	if (!IS_ERR(panel)) {
[ ... ]
> -	} else {
> -		next_bridge = of_drm_find_and_get_bridge(remote);
> -		if (!next_bridge)
> -			ret = -EINVAL;
> -	}
> +	next_bridge = of_drm_find_and_get_bridge(remote);
> +	if (!next_bridge)
> +		ret = -EINVAL;
>  
>  	of_node_put(remote);
>  

[Severity: Critical]
This isn't a bug introduced by this patch, but there is a pre-existing double
free and use-after-free issue in this driver related to the TE IRQ handling,
which is called from the error path of samsung_dsim_host_attach().

When a CMD mode device attaches, samsung_dsim_register_te_irq() allocates
dsi->te_gpio using devm_gpiod_get_optional(). If it fails later, or when
samsung_dsim_unregister_te_irq() is called on detach, the code manually calls
gpiod_put(dsi->te_gpio) without clearing the pointer.

Because the GPIO was allocated with devres, won't this manual gpiod_put()
cause a double free when the device unbinds and devres cleans up?

drivers/gpu/drm/bridge/samsung-dsim.c:samsung_dsim_unregister_te_irq() {
	if (dsi->te_gpio) {
		free_irq(gpiod_to_irq(dsi->te_gpio), dsi);
		gpiod_put(dsi->te_gpio);
	}
}

This manual cleanup is also present in the error path of
samsung_dsim_register_te_irq():

drivers/gpu/drm/bridge/samsung-dsim.c:samsung_dsim_register_te_irq() {
	...
	if (ret) {
		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
		gpiod_put(dsi->te_gpio);
		return ret;
	}
	...
}

Additionally, since samsung_dsim_unregister_te_irq() leaves a stale pointer
in dsi->te_gpio, could this lead to a use-after-free?

If a VIDEO mode device subsequently attaches, it skips registering the TE IRQ
and leaves the stale pointer in place. When it detaches,
samsung_dsim_host_detach() unconditionally calls
samsung_dsim_unregister_te_irq(), which would call gpiod_to_irq() on the
already freed pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11