Re: [PATCH v2 1/3] pinctrl: airoha: limit GPIO interrupts to interrupt-capable pins

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch fails to properly bound the inner interrupt loop to `num_irq_pins`, leaving it vulnerable to falsely claiming shared interrupts.

Pre-existing issues:
- [High] Use-After-Free in the interrupt handler during device unbind due to incorrect devres registration order.
- [High] The interrupt handler unconditionally clears the status register after processing, creating a race condition that permanently drops edge interrupts.
--

commit a41a436d88572aa867cd2449961e1f380a5c1d31
Author: Ahmed Naseef <naseefkm@gmail.com>

pinctrl: airoha: limit GPIO interrupts to interrupt-capable pins

The driver assumes that every one of the AIROHA_NUM_PINS GPIOs can raise
an interrupt. That holds for the SoCs supported so far, but not for every
member of the family: on EN7528 only GPIO0-GPIO15 are wired to the
interrupt controller.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index f505a3f69c5d1..52a768c859b71 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
[ ... ]
> @@ -304,9 +307,11 @@ static irqreturn_t airoha_irq_handler(int irq, void *data)
>  {
>  	struct airoha_pinctrl *pinctrl = data;
>  	bool handled = false;
> +	unsigned int nbanks;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(irq_status_regs); i++) {
> +	nbanks = DIV_ROUND_UP(pinctrl->num_irq_pins, AIROHA_PIN_BANK_SIZE);
> +	for (i = 0; i < nbanks; i++) {
>  		struct gpio_irq_chip *girq = &pinctrl->gpiochip.irq;
>  		u32 regmap;
>  		unsigned long status;

[Severity: Medium]
Will this fail to bound the inner interrupt loop if num_irq_pins is not a
multiple of 32?

If num_irq_pins is, for example, 16 for EN7528, the inner loop still iterates
up to 32:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_handler() {
    ...
		status = regmap;
		for_each_set_bit(irq, &status, AIROHA_PIN_BANK_SIZE) {
			u32 offset = irq + i * AIROHA_PIN_BANK_SIZE;

			generic_handle_domain_irq(girq->domain, offset);
    ...
}

If hardware sets any status bits beyond num_irq_pins (like raw floating GPIO
state), they will be processed. generic_handle_domain_irq() will return
-EINVAL but handled will be set to true, falsely claiming the interrupt.

[Severity: High]
This is a pre-existing issue, but does this unconditional clear drop hardware
edge interrupts?

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_handler() {
    ...
		for_each_set_bit(irq, &status, AIROHA_PIN_BANK_SIZE) {
			u32 offset = irq + i * AIROHA_PIN_BANK_SIZE;

			generic_handle_domain_irq(girq->domain, offset);
			regmap_write(pinctrl->regmap,
				     pinctrl->gpio_regs->status[i], BIT(irq));
		}
    ...
}

When an edge interrupt occurs, generic_handle_domain_irq() correctly calls
the driver's .irq_ack callback to clear the latch before running the device
action handler.

If a new edge arrives during the action execution, the hardware latches it
again. However, when generic_handle_domain_irq() returns, the handler
unconditionally executes a redundant regmap_write() to clear the status bit a
second time, blindly wiping out any newly latched edge interrupt before it can
be processed.

[ ... ]
> @@ -362,6 +383,7 @@ static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,
>  
>  	girq->default_type = IRQ_TYPE_NONE;
>  	girq->handler = handle_bad_irq;
> +	girq->init_valid_mask = airoha_gpio_init_valid_mask;
>  	gpio_irq_chip_set_chip(girq, &airoha_gpio_irq_chip);
>  
>  	irq = platform_get_irq(pdev, 0);

[Severity: High]
This is a pre-existing issue, but can this devres registration order create a
use-after-free window during device unbind?

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinctrl_add_gpiochip() {
    ...
	err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
				dev_name(dev), pinctrl);
	if (err)
		return err;

	return devm_gpiochip_add_data(dev, gc, pinctrl);
}

The driver registers the parent IRQ with devm_request_irq() before registering
the gpiochip. During unbind, devres runs in reverse, calling gpiochip_remove()
(which destroys and frees girq->domain) before calling free_irq().

Because IRQF_SHARED is used, the handler remains active on the shared line.
If a shared interrupt fires in this window, airoha_irq_handler() runs and
dereferences the freed girq->domain pointer in generic_handle_domain_irq().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787931313.git.naseefkm@gmail.com?part=1