Re: [PATCH v3 6/7] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Uninitialized raw spinlock `data->lock` triggers a BUG/panic.
- [High] Chained IRQ handlers and IRQ domain are leaked on probe failure and driver unbind, causing Use-After-Free.
- [High] Type mismatch in `rtd1625_gpio_set_debounce()` causes memory corruption and panic.
- [High] Missing base offset in `rtd1625_gpio_set_debounce()` corrupts interrupt status registers.
- [High] Race condition in IRQ mapping leads to a NULL pointer dereference.
- [High] Incorrect hardware mask in `GPIO_REGMAP_GET_OP` breaks GPIO input value reading.
- [Medium] Unprotected state modifications via `gpiochip_disable_irq()` in `.irq_mask`.
--

commit 2806fbf586d5ca5b8f36cb1b33138f23dd2f5b19
Author: Tzuyi Chang <tychang@realtek.com>

gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

Add support for the GPIO controller found on Realtek DHC RTD1625 SoCs.

> diff --git a/drivers/gpio/gpio-rtd1625.c b/drivers/gpio/gpio-rtd1625.c
> new file mode 100644
> index 0000000000000..0eae4bb5577d9
> --- /dev/null
> +++ b/drivers/gpio/gpio-rtd1625.c

[ ... ]

> +static int rtd1625_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
> +				  unsigned int base, unsigned int offset, unsigned int *reg,
> +				  unsigned int *mask)
> +{
> +	/* Each GPIO has its own dedicated 32-bit register */
> +	*reg = base + offset * 4;
> +
> +	switch (op) {

[ ... ]

> +	case GPIO_REGMAP_SET_DIR_WREN_OP:
> +		*mask = RTD1625_GPIO_WREN(RTD1625_GPIO_DIR);
> +		break;
> +	case GPIO_REGMAP_GET_OP:
> +	case GPIO_REGMAP_GET_DIR_OP:
> +		*mask = RTD1625_GPIO_DIR;

GPIO_REGMAP_GET_OP is mapped to RTD1625_GPIO_DIR. Will this return the
direction configuration instead of the actual physical logic level of the pin
(RTD1625_GPIO_IN) when calling gpio_get_value()?

> +		break;
> +	default:
> +		*mask = 0;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned int rtd1625_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +					      unsigned int debounce)
> +{
> +	struct rtd1625_gpio *data = gpiochip_get_data(chip);

gpiochip_get_data(chip) returns a pointer to struct gpio_regmap when using the
gpio-regmap framework, not struct rtd1625_gpio.

Will dereferencing data->regmap later in this function cause a crash? Should
this use gpio_regmap_get_drvdata(gpiochip_get_data(chip)) like the other
functions?

> +	u8 deb_val;
> +	u32 val;
> +
> +	switch (debounce) {

[ ... ]

> +	case 50000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_50MS;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	val = FIELD_PREP(RTD1625_GPIO_DEBOUNCE, deb_val) | RTD1625_GPIO_DEBOUNCE_WREN;
> +	regmap_write(data->regmap, GPIO_CONTROL(offset), val);

Is data->info->base_offset missing from the register calculation here?

Writing to GPIO_CONTROL(offset) without the base offset will overwrite the
gpa_offset and gpda_offset interrupt status registers instead of the target
GPIO control register.

> +
> +	return 0;
> +}

[ ... ]

> +static void rtd1625_gpio_disable_irq(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 irq_type = irqd_get_trigger_type(d);
> +	struct rtd1625_gpio *data;
> +	struct gpio_regmap *gpio;
> +
> +	gpio = gpiochip_get_data(gc);
> +	data = gpio_regmap_get_drvdata(gpio);
> +
> +	if (irq_type & IRQ_TYPE_EDGE_BOTH)
> +		rtd1625_gpio_disable_edge_irq(data, hwirq);
> +	else if (irq_type & IRQ_TYPE_LEVEL_MASK)
> +		rtd1625_gpio_disable_level_irq(data, hwirq);
> +
> +	gpiochip_disable_irq(gc, hwirq);

This function is registered as the .irq_mask callback. Since the core
temporarily masks level-triggered interrupts during processing, will calling
gpiochip_disable_irq() clear GPIOD_FLAG_IRQ_IS_ENABLED?

If so, could this inadvertently allow a concurrent thread to bypass protections
(e.g. gpiod_direction_output()) and change the GPIO direction to output while
the interrupt is actively firing?

> +}

[ ... ]

> +static int rtd1625_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct rtd1625_gpio *data = domain->host_data;
> +
> +	irq_set_chip_data(irq, data->gpio_chip);

Could data->gpio_chip be NULL here?

The IRQ domain is added in rtd1625_gpio_probe() before data->gpio_chip is
populated by gpio_regmap_get_gpiochip(). If an interrupt is mapped
concurrently during probe, setting NULL chip data will cause a NULL pointer
dereference later when retrieving the chip data.

> +
> +	irq_set_chip_and_handler(irq, &rtd1625_iso_gpio_irq_chip, handle_bad_irq);
> +
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}

[ ... ]

> +static int rtd1625_gpio_probe(struct platform_device *pdev)
> +{
> +	struct gpio_regmap_config config = {0};
> +	struct device *dev = &pdev->dev;
> +	struct gpio_regmap *gpio_reg;
> +	struct rtd1625_gpio *data;
> +	void __iomem *irq_base;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;

Is there a missing initialization for data->lock?

Since data is zero-initialized by devm_kzalloc(), acquiring this uninitialized
raw spinlock via guard(raw_spinlock_irqsave)(&data->lock) in the IRQ callbacks
will trigger a kernel panic when lock debugging or PREEMPT_RT is enabled.

> +
> +	data->info = device_get_match_data(dev);
> +	if (!data->info)
> +		return -EINVAL;

[ ... ]

> +	data->domain = irq_domain_add_linear(dev->of_node,
> +					     data->info->num_gpios,
> +					     &rtd1625_gpio_irq_domain_ops,
> +					     data);
> +	if (!data->domain)
> +		return -ENOMEM;
> +
> +	ret = rtd1625_gpio_setup_irq(pdev, data);
> +	if (ret) {
> +		irq_domain_remove(data->domain);
> +		return ret;
> +	}
> +
> +	config.irq_domain = data->domain;
> +	config.drvdata = data;
> +	platform_set_drvdata(pdev, data);
> +
> +	gpio_reg = devm_gpio_regmap_register(dev, &config);
> +	if (IS_ERR(gpio_reg)) {
> +		irq_domain_remove(data->domain);
> +		return PTR_ERR(gpio_reg);
> +	}

If devm_gpio_regmap_register() fails, or when the driver is later unbound
(since there is no .remove callback), data is automatically freed by devres.

However, rtd1625_gpio_setup_irq() configures chained handlers on the parent
IRQs which are not cleaned up. Will this cause a use-after-free if the parent
hardware interrupt fires after cleanup? Also, is the irq_domain leaked on
driver unbind?

> +
> +	data->gpio_chip = gpio_regmap_get_gpiochip(gpio_reg);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=6