Re: [PATCH v12 08/11] gpiolib: request ACPI GPIO events for external IRQ domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: inverted irqchip teardown order in gpio_regmap_unregister()
--

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

gpiolib: request ACPI GPIO events for external IRQ domains

This commit updates gpiochip_irqchip_add_domain() to call
acpi_gpiochip_request_interrupts() so that GPIO-signaled ACPI _AEI events are
requested when a gpio_chip attaches an external IRQ domain.

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e5fb60111151f..63c51d9ee0bf5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2353,7 +2353,15 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc)
>  int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
>  				struct irq_domain *domain)
>  {
> -	return gpiochip_irqchip_add_allocated_domain(gc, domain, true);
> +	int ret;
> +
> +	ret = gpiochip_irqchip_add_allocated_domain(gc, domain, true);
> +	if (ret)
> +		return ret;
> +
> +	acpi_gpiochip_request_interrupts(gc);

[Severity: High]
Can calling acpi_gpiochip_request_interrupts() here trigger a use-after-free
on gc->irq.domain and a NULL pointer dereference on irqd during teardown in
gpio_regmap_unregister()?

In drivers/gpio/gpio-regmap.c, gpio_regmap_register() is the caller of
gpiochip_irqchip_add_domain(). When config->regmap_irq_chip is set, it
registers the gpio_chip first and then creates the regmap IRQ domain:

drivers/gpio/gpio-regmap.c:gpio_regmap_register() {
	...
	ret = gpiochip_add_data(chip, gpio);
	if (ret)
		goto err_free_bitmap;

#ifdef CONFIG_REGMAP_IRQ
	if (config->regmap_irq_chip) {
		gpio->regmap_irq_line = config->regmap_irq_line;
		ret = regmap_add_irq_chip_fwnode(dev_fwnode(config->parent), config->regmap,
						 config->regmap_irq_line, config->regmap_irq_flags,
						 0, config->regmap_irq_chip, &gpio->irq_chip_data);
		if (ret)
			goto err_remove_gpiochip;

		irq_domain = regmap_irq_get_domain(gpio->irq_chip_data);
	} else
#endif
	irq_domain = config->irq_domain;

	if (irq_domain) {
		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
		...
	}
}

With this change, gpiochip_irqchip_add_domain() calls
acpi_gpiochip_request_interrupts(gc), which walks _AEI resources, marks each
event descriptor with GPIOD_FLAG_USED_AS_IRQ via gpiochip_lock_as_irq(), maps
the virtual IRQ in gc->irq.domain, and requests a threaded IRQ.

However, during teardown, gpio_regmap_unregister() destroys the regmap
irqchip before removing the gpio_chip:

drivers/gpio/gpio-regmap.c:gpio_regmap_unregister() {
#ifdef CONFIG_REGMAP_IRQ
	if (gpio->irq_chip_data)
		regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
#endif

	gpiochip_remove(&gpio->gpio_chip);
	...
}

In drivers/base/regmap/regmap-irq.c, regmap_del_irq_chip() disposes all
mapped virtual IRQs via irq_dispose_mapping(virq) while the ACPI event
interrupts are still requested, and then frees d->domain via
irq_domain_remove(d->domain), leaving gpio->gpio_chip.irq.domain dangling.

Immediately after that, gpiochip_remove() runs:

gpiochip_remove()
  -> gpiochip_free_remaining_irqs()
    -> gpiod_free_irqs()
      -> gpiod_to_irq()
        -> gpiochip_to_irq()
          -> irq_create_mapping(gc->irq.domain, offset)

Because the ACPI event descriptor still has GPIOD_FLAG_USED_AS_IRQ set,
gpiod_free_irqs() calls gpiod_to_irq(), which dereferences the freed
gc->irq.domain. Furthermore, in drivers/gpio/gpiolib.c:gpiod_free_irqs():

	int irq = gpiod_to_irq(desc);
	struct irq_desc *irqd = irq_to_desc(irq);
	void *cookie;

	for (;;) {
		scoped_guard(mutex, &irqd->request_mutex) {

if gpiod_to_irq() fails and returns a negative error, irq_to_desc(irq)
returns NULL and &irqd->request_mutex dereferences a NULL pointer, followed
by gpiochip_irqchip_remove() -> acpi_gpiochip_free_interrupts() calling
free_irq() on an already-disposed virtual IRQ.

Should gpio_regmap_unregister() call gpiochip_remove(&gpio->gpio_chip) before
regmap_del_irq_chip() so that the ACPI event interrupts and gpio_chip are
torn down before the underlying regmap irq_domain is removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-cv75-gpio-v12-0-c22555400e18@ambarella.com?part=8