Re: [PATCH v9 11/15] 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: pl061: UAF due to LIFO devres cleanup ordering
- [High] gpio: pl061: Missing GPIOCHIP_IRQ_RESOURCE_HELPERS allows HW conflict
- [Medium] gpio: pl061: Missing synchronize_irq() causes UAF on module unload
--

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

gpio: pl061: switch data path to gpio-regmap

This commit migrates the PL061 driver to use gpio-regmap for its data path,
handling the PL061 quirk where output values must be written after a direction
change. It sets up a custom irqchip and domain attached via gpio-regmap, and
modifies the interrupt handler to appropriately check regmap reads.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index c821a5117eede..08d45f8923c2a 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> -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,
> @@ -322,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 the omission of GPIOCHIP_IRQ_RESOURCE_HELPERS from this irq_chip
definition detach it from gpiolib's resource locking?

Without these helpers, gpiochip_lock_as_irq() is never called when a pin
is requested as an IRQ, so GPIOD_FLAG_USED_AS_IRQ is not set. Could this
allow userspace to accidentally reconfigure an active IRQ pin as an output,
causing hardware conflicts?

> +
> +static int pl061_irq_domain_map(struct irq_domain *d, unsigned int virq,
[ ... ]
> +static void pl061_remove_irq(void *data)
> +{
> +	struct pl061 *pl061 = data;
> +
> +	irq_set_chained_handler_and_data(pl061->parent_irq, NULL, NULL);

[Severity: Medium]
Is a call to synchronize_irq() needed here before proceeding?

While the chained handler is cleared, there appears to be no synchronization
with in-flight handlers. If pl061_irq_handler() is running concurrently
on another CPU, it might still be executing when we proceed.

> +
> +	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: Medium]
If pl061_irq_handler() is still running, could it access this freed domain
when it calls generic_handle_domain_irq(pl061->irq_domain, offset)?

> +}
[ ... ]
> +static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> +{
[ ... ]
> +	config.parent = dev;
> +	config.regmap = pl061->regmap;
> +	config.ngpio = data->ngpio;
> +	config.reg_dat_base = GPIO_REGMAP_ADDR(regs->dat);
> +	config.reg_set_base = GPIO_REGMAP_ADDR(regs->dat);
> +	config.reg_dir_out_base = GPIO_REGMAP_ADDR(regs->dir);
> +	config.reg_mask_xlate = data->reg_mask_xlate;
> +	config.quirks = data->quirks;
> +	config.irq_domain = pl061->irq_domain;
> +	config.drvdata = pl061;
> +
> +	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]
Can this devres action registration order cause a use-after-free during
driver unbind?

Since devm_add_action_or_reset() is called after devm_gpio_regmap_register(),
devres will execute pl061_remove_irq() first during teardown because it uses
LIFO ordering.

This means pl061_remove_irq() will run and free pl061->irq_domain, but
the gpiochip is still registered. When gpio_regmap subsequently unregisters
the gpiochip, gpiochip_remove() will eventually call
gpiochip_free_remaining_irqs(), which will attempt to use the already-freed
irq.domain pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-cv75-v5-v9-0-486cc289a4b8@ambarella.com?part=11