Re: [PATCH v7 14/34] pinctrl: airoha: fix potential kenel panic in IRQ handling code

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared parent IRQ is requested before the gpiochip and its IRQ domain are fully initialized, creating race conditions that lead to NULL pointer dereferences on probe and Use-After-Free during teardown.
- [High] The driver hardcodes `handle_simple_irq` for all interrupts and manually ACKs status bits after handling, causing race conditions that permanently lose edge interrupts.
- [High] The irq_chip callbacks call regmap functions on a syscon regmap, attempting to acquire a sleepable spinlock while holding a raw spinlock.
--

--- Patch [14]: [PATCH v7 14/34] pinctrl: airoha: fix potential kenel panic in IRQ handling code ---
commit 4e32db00b413a54d784b7251323863762cc5e739
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

    pinctrl: airoha: fix potential kenel panic in IRQ handling code
    
    This commit fixes invalid pointer initializations for gpiochip and pinctrl
    variables in the irq callback functions. Previously, these pointers were
    derived incorrectly, leading to dangerous memory accesses. The patch updates
    the code to correctly obtain the required pointers.

> pinctrl: airoha: fix potential kenel panic in IRQ handling code

This isn't a bug, but there's a typo (kenel) in the subject line.

[ ... ]

> So any access to the data pointed by gpiochip and pinctrl variables is
> extremelly dangerous.

This isn't a bug, but there's a typo (extremelly) here.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index fcf498f7720c9..6b881ab56d9c3 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -2606,14 +2605,12 @@ static void airoha_irq_unmask(struct irq_data *data)
>  
>  static void airoha_irq_mask(struct irq_data *data)
>  {
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(gc);
> +	struct airoha_pinctrl_gpiochip *gpiochip = &pinctrl->gpiochip;
>  	u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
>  
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->level[index], mask);

[Severity: High]
This is a pre-existing issue, but on real-time kernels, wouldn't this result
in a scheduling while atomic bug?

The airoha_irq_mask() and airoha_irq_unmask() callbacks execute in atomic
hardirq context holding a raw_spinlock_t. However, they call
regmap_clear_bits() and regmap_update_bits() which attempt to acquire the
syscon regmap's lock.

Since the regmap from airoha_pinctrl_probe() uses a standard spinlock_t
(which becomes a sleepable rt_mutex on real-time kernels), could acquiring it
here trigger an invalid locking sequence?

[ ... ]

While reviewing this, I noticed a couple of other pre-existing regressions
in the driver's interrupt handling code:

[Severity: High]
This isn't a bug introduced by this patch, but does the devres cleanup order
create a use-after-free or null pointer dereference risk?

In airoha_pinctrl_add_gpiochip(), the parent interrupt is requested before the
gpiochip is added:

	err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
				dev_name(dev), pinctrl);
	if (err) {
		dev_err(dev, "error requesting irq %d: %d\n", irq, err);
		return err;
	}

	return devm_gpiochip_add_data(dev, gc, pinctrl);

Due to devres ordering (first-in, last-out), the domain will be destroyed
before the parent interrupt is freed during device unbind or if a later probe
step fails.

If an interrupt fires during this teardown window, wouldn't
airoha_irq_handler() dereference the freed girq->domain pointer here?

			generic_handle_irq(irq_find_mapping(girq->domain,
							    offset));

Could the request be moved after the gpiochip is added to ensure safe
teardown?

[Severity: High]
This is a pre-existing issue, but could this logic permanently lose concurrent
edge events?

In airoha_pinctrl_add_gpiochip(), the driver hardcodes handle_simple_irq for
all interrupts, regardless of their type:

	girq->handler = handle_simple_irq;
	gpio_irq_chip_set_chip(girq, &airoha_gpio_irq_chip);

And in airoha_irq_handler(), the hardware status bit is manually cleared
after handling the event:

			generic_handle_irq(irq_find_mapping(girq->domain,
							    offset));
			regmap_write(pinctrl->regmap,
				     pinctrl->gpiochip.status[i], BIT(irq));

If a second edge event arrives while the child handler is executing, wouldn't
the manual regmap_write unconditionally clear the status bit and cause the
new edge event to be discarded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727074234.3761170-1-mikhail.kshevetskiy@iopsys.eu?part=14