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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] gpio: UAF of irq_domain during devm teardown due to LIFO ordering
- [High] gpio: Missing GPIOCHIP_IRQ_RESOURCE_HELPERS allows GPIO/IRQ conflicts
- [High] gpio: Missing synchronize_irq() causes UAF on chained handler removal
--

commit b485edbbc0d9055aa12255ec0a09e3fe648808bb
Author: Long Zhao <longzhao@ambarella.com>

    gpio: pl061: switch data path to gpio-regmap
    
    Move get, set and direction onto gpio-regmap. ARM PL061 sets
    GPIO_REGMAP_QUIRK_SET_AFTER_DIR so the output value is written
    after the direction change.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index b7632b30a9098..ba3350a8971c8 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -293,29 +227,28 @@ static void pl061_irq_unmask(struct irq_data *d)
[ ... ]
> -static const struct irq_chip pl061_irq_chip = {
> +static const struct irq_chip pl061_irqchip = {
> +	.name			= "pl061",
>  	.irq_ack		= pl061_irq_ack,
>  	.irq_mask		= pl061_irq_mask,
>  	.irq_unmask		= pl061_irq_unmask,
> @@ -323,67 +256,187 @@ 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: High]
Does this code omit the GPIOCHIP_IRQ_RESOURCE_HELPERS macro? 

Without the .irq_request_resources and .irq_release_resources callbacks it
provides, gpiolib won't be notified when an IRQ is requested, and
gpiochip_lock_as_irq() is skipped.

Could this allow the GPIO line to be concurrently configured as an output
while it is actively firing interrupts?

[ ... ]
> +static void pl061_remove_irq(void *data)
> +{
> +	struct pl061 *pl061 = data;
> +
> +	irq_set_chained_handler_and_data(pl061->parent_irq, NULL, NULL);

[Severity: High]
Can a concurrent interrupt handler still be executing here?

This replaces the flow handler but doesn't wait for currently executing
handlers to finish like synchronize_irq() would.

> +
> +	for (unsigned int i = 0; i < pl061->data->ngpio; i++) {
> +		unsigned int virq;
> +
> +		virq = irq_find_mapping(pl061->irq_domain, i);
> +		if (virq)
> +			irq_dispose_mapping(virq);
> +	}
> +
> +	irq_domain_remove(pl061->irq_domain);

[Severity: High]
Could the lack of synchronization against concurrent interrupts lead to a
use-after-free here?

If pl061_irq_handler() is concurrently executing on another CPU, it might call
generic_handle_domain_irq(pl061->irq_domain, ...) and dereference the
domain after it has been freed.

> +}
[ ... ]
> +	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]
Because devm executes actions in reverse (LIFO) order, pl061_remove_irq() will
execute before the gpio_regmap is unregistered.

Will this destroy the irq_domain while the gpio_chip is still active?

If so, when the gpio_chip is later unregistered, gpiochip_remove() might
dereference the freed domain pointer when attempting to tear down requested
IRQs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-cv75-gpio-v1-0-5836bd438423@ambarella.com?part=6