Re: [PATCH v10 7/8] gpio: pl061: switch data path to gpio-regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: pl061: irq_domain use-after-free on teardown due to devres LIFO order
- [Medium] gpio: pl061: Missing IRQ resource helpers allows concurrent GPIO direction changes
--

commit 754bf2316c41a0da3dbb26f8d31b1ba0d0a17c9d
Author: Long Zhao <longzhao@ambarella.com>
Subject: gpio: pl061: switch data path to gpio-regmap

This patch moves the PL061 get, set, and direction operations to use
gpio-regmap. It keeps a custom irqchip and irq domain, creating the domain
with private data and attaching it via gpio-regmap.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 5c6d4209dc8e7..3822faa3e2768 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -325,67 +284,159 @@ static const struct irq_chip pl061_irq_chip = {
>  	.irq_set_wake		= pl061_irq_set_wake,
>  	.irq_print_chip		= pl061_irq_print_chip,
>  	.flags			= IRQCHIP_IMMUTABLE,
> -	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

[Severity: Medium]
Does removing GPIOCHIP_IRQ_RESOURCE_HELPERS leave the interrupt pins
unprotected against concurrent GPIO direction changes?

Without the .irq_request_resources and .irq_release_resources callbacks,
the gpiolib gpiochip_lock_as_irq() helper is never invoked when an interrupt
is requested. This means userspace or other subsystems could inadvertently
reconfigure an actively interrupting pin as an output.

While omitting the default helpers avoids crashing since
irq_data_get_irq_chip_data() now returns struct pl061 * rather than
struct gpio_chip *, are custom callbacks needed here to properly fetch
the gpio_chip and lock the line?

[ ... ]
> +	gpio_regmap = devm_gpio_regmap_register(dev, &config);
> +	if (IS_ERR(gpio_regmap)) {
> +		if (pl061->irq_domain)
> +			irq_domain_remove(pl061->irq_domain);
> +		return PTR_ERR(gpio_regmap);
> +	}
> +
> +	if (irq) {
> +		ret = devm_add_action_or_reset(dev, pl061_remove_irq, pl061);

[Severity: High]
Will this devres registration order cause a use-after-free during device
teardown?

The driver registers the GPIO chip via devm_gpio_regmap_register(), which
queues a devres action to unregister the chip via gpiochip_remove().

Later, devm_add_action_or_reset() is called to register pl061_remove_irq().

Since devres actions are unwound in LIFO order upon device unbind or probe
failure, pl061_remove_irq() will execute first and free the irq_domain:

pl061_remove_irq()
    ...
    irq_domain_remove(pl061->irq_domain);

Afterward, the gpio_regmap teardown action runs and calls gpiochip_remove().
This function iterates over active IRQs and dereferences the domain to
dispose of mappings:

gpiochip_remove()
    ... accesses gc->irq.domain ...

Since the domain was already freed by pl061_remove_irq(), does this result
in a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-cv75-gpio-v10-0-3fe3eb315a55@ambarella.com?part=7