Re: [PATCH v6 08/17] pinctrl: airoha: split driver on shared code and SoC specific drivers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dynamic allocation of `irq_chip` violates `IRQCHIP_IMMUTABLE` W^X protection and lacks `GPIOCHIP_IRQ_RESOURCE_HELPERS`.

Pre-existing issues:
- [High] Race condition and Use-After-Free during teardown due to early `devm_request_irq()` registration.
- [High] Missing `gpiochip_disable_irq()` and `gpiochip_enable_irq()` calls in `irq_chip` callbacks breaks pin locking.
--

commit 39c02a07ea03e1c882e3d925b659bdbc86446b63
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

pinctrl: airoha: split driver on shared code and SoC specific drivers

Split combined an7581/an7583 source file into shared pinctrl code and SoC
specific pinctrl drivers. The initialization code was adapted to work properly
for shared and SoC specific cases.

> diff --git a/drivers/pinctrl/airoha/airoha-common.h b/drivers/pinctrl/airoha/airoha-common.h
> index bff162062b066..9a202d5bb4b13 100644
> --- a/drivers/pinctrl/airoha/airoha-common.h
> +++ b/drivers/pinctrl/airoha/airoha-common.h
> @@ -150,9 +150,12 @@ struct airoha_pinctrl {
>  	struct regmap *regmap;
>  
>  	struct airoha_pinctrl_gpiochip gpiochip;
> +	struct irq_chip gpio_irq_chip;
>  };

[Severity: High]
Does dynamically allocating the irq_chip struct here defeat the purpose of
IRQCHIP_IMMUTABLE?

By dynamically allocating the irq_chip structure within airoha_pinctrl, it
resides in writable heap memory rather than read-only .rodata, which bypasses
the W^X protection.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index e67c19df9f457..543475d23fde3 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
[ ... ]
> @@ -2552,9 +256,16 @@ static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,
>  	gc->base = -1;
>  	gc->ngpio = AIROHA_NUM_PINS;
>  
> +	irq_chip->name = "airoha-gpio-irq";
> +	irq_chip->irq_unmask = airoha_irq_unmask;

[Severity: High]
This is a pre-existing issue, but does airoha_irq_unmask() fail to invoke
gpiochip_enable_irq()?

When using IRQCHIP_IMMUTABLE, airoha_irq_unmask() must manually call
gpiochip_enable_irq() and airoha_irq_mask() must call gpiochip_disable_irq().

Without these calls, gpiolib never sets the GPIOD_FLAG_IRQ_IS_ENABLED bit.
Because this bit is missing, gpiod_direction_output() will not realize the pin
is actively used as an interrupt, allowing it to be successfully reconfigured
into an output pin, which can cause a hardware short-circuit.

> +	irq_chip->irq_mask = airoha_irq_mask;
> +	irq_chip->irq_mask_ack = airoha_irq_mask;
> +	irq_chip->irq_set_type = airoha_irq_type;
> +	irq_chip->flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_IMMUTABLE;

[Severity: High]
Should GPIOCHIP_IRQ_RESOURCE_HELPERS be included here?

Without hooking .irq_request_resources, gpiolib cannot set
GPIOD_FLAG_USED_AS_IRQ. This breaks pin direction locking and can allow
userspace or another driver to reconfigure an active IRQ pin as an output,
potentially causing a hardware short-circuit.

> +
>  	girq->default_type = IRQ_TYPE_NONE;
>  	girq->handler = handle_simple_irq;
> -	gpio_irq_chip_set_chip(girq, &airoha_gpio_irq_chip);
> +	gpio_irq_chip_set_chip(girq, irq_chip);
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)

[Severity: High]
This is a pre-existing issue, but can the sequence of allocations here cause
a use-after-free during teardown?

Since devm_request_irq() is called with IRQF_SHARED before adding the
gpiochip and its irqdomain later in airoha_pinctrl_add_gpiochip(), the devm
cleanup will destroy the gpiochip and irqdomain before unhooking the
interrupt.

If a shared interrupt fires from another device on the same IRQ line during
removal or probe failure, airoha_irq_handler() will dereference girq->domain
after it has been freed.

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