Re: [PATCH 7/7] gpio: pl061: add Ambarella register-layout variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: pl061: regmap cache seeded with physical pin levels on first write
- [High] gpio: UAF of irq_domain in gpiochip_remove() due to devres ordering
--

commit 35420eba05adeb99d5c7ba4b27a388ab6107e2d2
Author: Long Zhao <longzhao@ambarella.com>
Subject: gpio: pl061: add Ambarella register-layout variant

Add a 32-bit, 32-line layout with DAT/DIR/IS/IBE/IEV/IE/MIS/IC plus
MASK and ENABLE. Probe unmasks the bank and enables the controller.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index ba3350a8971c8..ccadeb1183058 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -318,6 +330,17 @@ static bool pl061_arm_volatile_reg(struct device *dev, unsigned int reg)
[ ... ]
> +static bool pl061_ambarella_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case AMBARELLA_MIS:
> +	case AMBARELLA_IC:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
[ ... ]
> +static const struct regmap_config pl061_ambarella_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = AMBARELLA_ENABLE,
> +	.volatile_reg = pl061_ambarella_volatile_reg,
> +	.cache_type = REGCACHE_FLAT_S,
> +	.fast_io = true,
> +	.use_raw_spinlock = true,
> +};

[Severity: High]
Does this configuration cause the regmap cache to be seeded with physical
pin levels on the first write?

Because AMBARELLA_DAT is not marked as volatile in
pl061_ambarella_volatile_reg() and the regmap_config lacks .reg_defaults,
the cache is completely empty at probe. On the first call to gpio_set(),
the regmap core experiences a cache miss in _regmap_update_bits() and
falls back to a physical MMIO read via _regmap_read().

This populates the regmap cache with the physical input levels of all pins
on the bank. The single requested bit is then modified, and the full 32-bit
register is written back to hardware.

If any output pin was heavily loaded or externally driven during this
initial read, could its false physical state be latched and permanently
driven as an output, directly causing the glitch the patch aimed to prevent?

[ ... ]
> @@ -348,6 +395,13 @@ static const struct pl061_drvdata pl061_arm = {
>  	.reg_mask_xlate		= pl061_arm_reg_mask_xlate,
>  };
>  
> +static const struct pl061_drvdata pl061_ambarella = {
> +	.regmap_config		= &pl061_ambarella_regmap_config,
> +	.regs			= &pl061_ambarella_regs,
> +	.ngpio			= AMBARELLA_GPIO_NR,
> +	.clear_irq_on_type	= true,
> +};
> +
>  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  {

[Severity: High]
Does unbinding the driver trigger a use-after-free of the irq_domain in
gpiochip_remove() due to devres ordering?

Inside pl061_probe(), the devres action to remove the IRQ is registered
after the gpio_regmap:

    gpio_regmap = devm_gpio_regmap_register(dev, &config);
    ...
    if (irq) {
        ret = devm_add_action_or_reset(dev, pl061_remove_irq, pl061);
    ...

If a client or userspace holds a GPIO IRQ, unbinding the driver triggers
devres teardown in LIFO order. pl061_remove_irq() will run first, calling
irq_domain_remove(), which kfrees the irq_domain.

Next, devm_gpio_regmap_unregister() runs and calls gpiochip_remove().
This invokes gpiochip_free_remaining_irqs(), which calls gpiod_free_irqs()
and eventually gpiochip_to_irq():

drivers/gpio/gpiolib.c:gpiochip_to_irq() {
    struct irq_domain *domain = gc->irq.domain;
    ...
    return irq_create_mapping(domain, offset);
}

Because gc->irq.domain was already freed by pl061_remove_irq(), this causes
a use-after-free.

Can the devres registration order be adjusted to ensure the gpiochip is
removed before the irq_domain is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-cv75-gpio-v1-0-5836bd438423@ambarella.com?part=7