Re: [PATCH 04/11] gpio: Add Cortina Systems CS75xx GPIO driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] gpio: cs75xx: TOCTOU race overwrites IRQ trigger hardware state
- [Medium] gpio: cs75xx: BOTH_EDGES state corruption on invalid IRQ type
- [Medium] gpio: cs75xx: missing ngpios bounds check causes OOB read and shift UB
--

commit a5885a6ba82fad811f622ae265473cbbb66fe3ae
Author: Fil Dunsky <filipp.dunsky@gmail.com>

gpio: Add Cortina Systems CS75xx GPIO driver

This commit adds a driver for the GPIO banks of the Cortina Systems CS75xx
SoCs, handling data and direction registers via gpio-mmio and providing an
interrupt controller with level and edge trigger support.

> diff --git a/drivers/gpio/gpio-cs75xx.c b/drivers/gpio/gpio-cs75xx.c
> index 0000000000000..168af0e37ac20
> --- /dev/null
> +++ b/drivers/gpio/gpio-cs75xx.c
[ ... ]
> +static int cs75xx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct cs75xx_gpio *g = gpiochip_get_data(gc);
> +	u32 mask = BIT(irqd_to_hwirq(d));
> +	u32 lvl, edge;
> +
> +	guard(gpio_generic_lock_irqsave)(&g->chip);
> +
> +	lvl = readl(g->base + CS75XX_GPIO_LVL);
> +	edge = readl(g->base + CS75XX_GPIO_EDGE);
> +	g->both_edges &= ~mask;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
[ ... ]
> +	default:
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does this sequence cause a regression in the BOTH_EDGES state if an invalid
IRQ type is requested?

The both_edges bitmask is modified before validating the requested type. If
the request falls through to the default case (e.g., IRQ_TYPE_NONE) and returns
-EINVAL, the state mutation is already committed.

If the pin was previously operating in IRQ_TYPE_EDGE_BOTH mode, the interrupt
handler will no longer flip its polarity on subsequent interrupts, causing
a regression in the pin's functionality.

Could we defer modifying g->both_edges until after all validation succeeds?

[ ... ]
> +static int cs75xx_gpio_irq_reqres(struct irq_data *d)
> +{
[ ... ]
> +	guard(gpio_generic_lock_irqsave)(&g->chip);
> +	g->irq_used |= BIT(hwirq);

[Severity: Medium]
Could this result in undefined shift behavior if hwirq is 32 or greater?

If a Device Tree specifies an ngpios value greater than 32, this assignment
could invoke shift undefined behavior because BIT(hwirq) is assigned into
32-bit state variables.

While the current in-tree DT might not specify ngpios, should we clamp
gc->ngpio to 32 during probe to protect against malformed DT inputs?

[ ... ]
> +static void cs75xx_gpio_irq_handler(struct irq_desc *desc)
> +{
> +	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +	struct cs75xx_gpio *g = gpiochip_get_data(gc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long pending;
> +	unsigned int offset;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	pending = readl(g->base + CS75XX_GPIO_INT) &
> +		  readl(g->base + CS75XX_GPIO_IE);
> +
> +	for_each_set_bit(offset, &pending, gc->ngpio) {

[Severity: Medium]
Will this loop read out of bounds if gc->ngpio exceeds BITS_PER_LONG?

If the Device Tree specifies a larger ngpios value, iterating over a single
stack-allocated unsigned long pending with gc->ngpio could cause a stack
out-of-bounds read on 32-bit architectures (where unsigned long is 32 bits).

> +		if (g->both_edges & BIT(offset)) {
> +			guard(gpio_generic_lock)(&g->chip);
> +			cs75xx_gpio_set_polarity(g, BIT(offset));
> +		}

[Severity: High]
Can this lockless condition check expose a Time-Of-Check to Time-Of-Use
(TOCTOU) race that can overwrite the IRQ trigger hardware state?

The g->both_edges bitmask is checked locklessly before acquiring g->chip.lock.
A concurrent call to cs75xx_gpio_irq_set_type() on another CPU could:

1. Acquire the lock
2. Reconfigure the trigger type (clearing the bit in g->both_edges)
3. Update the hardware LVL and EDGE registers
4. Release the lock

The interrupt handler would then acquire the lock and execute
cs75xx_gpio_set_polarity(), which blindly reads and overwrites the LVL register
based on the current pin level. This would overwrite the new trigger
configuration just established by irq_set_type(), leaving the hardware in an
incorrect state.

Should we re-evaluate the both_edges condition after the lock is acquired, or
acquire the lock before performing the check?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-b4-cs75xx-s2-v1-0-8a7641b7b79c@gmail.com?part=4