Re: [PATCH v3 2/2] pinctrl: meson: restore non-sleeping GPIO access

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

New issues:
- [High] Setting `can_sleep = false` without enabling raw spinlocks in the regmap configuration will cause panics on PREEMPT_RT kernels when called from atomic context.

Pre-existing issues:
- [High] The driver leaks the registered `gpio_chip` and causes a use-after-free on device unbind due to the lack of devres management for the GPIO chip.
- [High] Concurrent probes will race to modify the global static `meson_regmap_config` structure, potentially leading to corrupted regmap initialization.
- [High] Setting a GPIO to output temporarily exposes the old driven value before setting the new value, causing an output glitch.
- [Medium] The return value of `regmap_read()` is ignored in `meson_gpio_get()`, potentially evaluating uninitialized stack memory.
--

commit b08edbdcf628b329dd9c5cf2f8f1fd105ba240d1
Author: Viacheslav Bocharov <[email protected]>

pinctrl: meson: restore non-sleeping GPIO access

Commit 28f240683871 ("pinctrl: meson: mark the GPIO controller as
sleeping") set gpio_chip.can_sleep = true to work around
gpio-shared-proxy holding a spinlock across a sleeping pinctrl config
path. That locking bug is now fixed in the shared-proxy itself ("gpio:
shared-proxy: always serialize with a sleeping mutex"), so the
controller-wide workaround is no longer needed; the meson GPIO
controller does not sleep.

[ ... ]

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 4507dc8b5563c..18295b15ecd9d 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -619,7 +619,7 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
>  	pc->chip.set = meson_gpio_set;
>  	pc->chip.base = -1;
>  	pc->chip.ngpio = pc->data->num_pins;
> -	pc->chip.can_sleep = true;
> +	pc->chip.can_sleep = false;

[Severity: High]
Does setting can_sleep to false without configuring the regmap to use raw
spinlocks cause panics on PREEMPT_RT kernels?

When atomic consumers like w1-gpio acquire a raw lock and disable IRQs, they
then call gpiod_set_value(). Since meson_regmap_config does not set
use_raw_spinlock = true, the MMIO regmap uses a standard spinlock_t which
sleeps on PREEMPT_RT. Taking a sleeping lock with interrupts disabled will
crash the system.

>  
>  	ret = gpiochip_add_data(&pc->chip, pc);

[Severity: High]
This is a pre-existing issue, but does this leak the registered gpio_chip and
cause a use-after-free on device unbind?

Because the platform driver lacks a .remove callback and uses
gpiochip_add_data() instead of devm_gpiochip_add_data(), the GPIO chip
remains registered after struct meson_pinctrl is freed by devres.

>  	if (ret) {
>  		dev_err(pc->dev, "can't add gpio chip %s\n",

drivers/pinctrl/meson/pinctrl-meson.c:meson_map_resource() {
    ...
	meson_regmap_config.max_register = resource_size(&res) - 4;
	meson_regmap_config.name = devm_kasprintf(pc->dev, GFP_KERNEL,
						  "%pOFn-%s", node,
						  name);
    ...
}

[Severity: High]
This is a pre-existing issue, but will concurrent probes race to modify the
global static meson_regmap_config structure?

Multiple Meson pinctrl instances probing concurrently can modify these fields
without synchronization, potentially leading to out-of-bounds register
accesses or use-after-free of strings.

drivers/pinctrl/meson/pinctrl-meson.c:meson_pinconf_set_output_drive() {
    ...
	ret = meson_pinconf_set_output(pc, pin, true);
	if (ret)
		return ret;

	return meson_pinconf_set_drive(pc, pin, high);
}

[Severity: High]
This is a pre-existing issue, but could setting the GPIO direction to output
before setting the desired output value cause a physical glitch on the GPIO
line?

During the window between the two regmap updates, the pin is driven with
whatever stale value was previously in the output register.

drivers/pinctrl/meson/pinctrl-meson.c:meson_gpio_get() {
    ...
	meson_calc_reg_and_bit(bank, gpio, MESON_REG_IN, &reg, &bit);
	regmap_read(pc->reg_gpio, reg, &val);

	return !!(val & BIT(bit));
}

[Severity: Medium]
This is a pre-existing issue, but is it possible for regmap_read() to fail
here?

If the register access fails, the return value is ignored. The uninitialized
stack variable val is then evaluated, resulting in non-deterministic GPIO
state being reported to consumers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic