Re: [PATCH 6.6.y 1/2] gpio: rockchip: teardown bugs and resource leaks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: rockchip: UAF in rockchip_gpio_to_irq due to dangling bank->domain
- [Medium] gpio: rockchip: clk_put() on prepared debounce clock triggers WARN_ON_ONCE
--

commit 02053f662558738041ac46d1ce4ccf500e2ba8c2
Author: Marco Scardovi <scardracs@disroot.org>

gpio: rockchip: teardown bugs and resource leaks

This commit addresses several teardown issues and resource leaks in the
driver's remove path and error handling. It registers a devm action to
release the debounce clock, clears the chained IRQ handler in remove(),
and removes the IRQ domain during driver teardown.

Link: https://patch.msgid.link/20260526171050.12785-3-scardracs@disroot.org

> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index ff9a4b8611d7f..e0e4f3ed5fdd4 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -629,10 +629,17 @@ static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
>  	return ret;
>  }
>  
> +static void rockchip_clk_put(void *data)
> +{
> +	struct clk *clk = data;
> +
> +	clk_put(clk);
> +}

[Severity: Medium]
Does this code trigger a warning if the debounce clock is still prepared
and enabled?

If a GPIO line is configured with debounce, it enables the clock:

rockchip_gpio_set_debounce() {
	...
	if (debounce)
		clk_prepare_enable(bank->db_clk);
	...
}

If the driver is unbound while debounce is still active, this devm action
calls clk_put() without unpreparing or disabling the clock first. This
triggers a warning in the clock framework because the active prepare count
is unbalanced.

[ ... ]

> @@ -773,6 +785,9 @@ static int rockchip_gpio_remove(struct platform_device *pdev)
>  {
>  	struct rockchip_pin_bank *bank = platform_get_drvdata(pdev);
>  
> +	irq_set_chained_handler_and_data(bank->irq, NULL, NULL);
> +	if (bank->domain)
> +		irq_domain_remove(bank->domain);

[Severity: High]
Does this cause a use-after-free if another thread tries to map an IRQ
during driver unbind?

The IRQ domain is freed before the GPIO chip is removed, and bank->domain
is never set to NULL. If a concurrent call to gpiod_to_irq() occurs:

rockchip_gpio_to_irq() {
	...
	if (!bank->domain)
		return -ENXIO;
	...
	virq = irq_create_mapping(bank->domain, offset);
	...
}

It bypasses the NULL check and accesses the freed domain memory.

Could this also cause a use-after-free during driver rebind?

If the driver is unbound and then bound again, rockchip_gpiolib_register()
calls gpiochip_add_data() before the new IRQ domain is allocated. A
concurrent call to gpiod_to_irq() during this probe window would see the
stale, dangling bank->domain pointer from the previous unbind and attempt
to use it.

>  	gpiochip_remove(&bank->gpio_chip);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008191824.98662-1-artem@trailofbits.com?part=1