Re: [PATCH] gpiolib: cdev: don't record a line direction the driver refused

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: memory leak in gpiod_set_array_value_complex() on error paths
- [High] gpio: non-atomic desc->flags update clobbers concurrent IRQ flags
--

commit 910242af9150f094f4676cb26fb8b58cc8edfa96
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

gpiolib: cdev: don't record a line direction the driver refused

The commit modifies gpio_v2_line_config_flags_to_desc_flags() to avoid setting
the GPIOD_FLAG_IS_OUT flag before the hardware direction is confirmed by the
driver. This fixes an issue where refused configurations would leave the line
erroneously reported as an output or input.

> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 5d53bfcdf7268..9bbee3a1f9c9c 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
>  {
>  	unsigned long flags = READ_ONCE(*flagsp);

[Severity: High]
Does this non-atomic read-modify-write clobber concurrent atomic updates to
desc->flags made by the IRQ subsystem?

If a driver or IRQ domain calls gpiochip_lock_as_irq() or
gpiochip_enable_irq() concurrently, they atomically update bits like
GPIOD_FLAG_USED_AS_IRQ:

	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);

Because the IRQ subsystem does not check GPIOD_FLAG_REQUESTED before using
the line and does not share locks with the cdev ioctl, these atomic updates
can happen between this READ_ONCE() and the subsequent
WRITE_ONCE(*flagsp, flags) at the end of this function.

Wouldn't the atomic updates be silently overwritten when this function writes
back the stale flags value?

Losing these flags disables the safety check in
gpiod_direction_output_nonotify(), which could allow user-space to force an
active interrupt line into output mode.

>  
> +	/*
> +	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
> +	 * only update it once the chip has accepted the direction.
> +	 */
>  	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
>  		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
>  
> -	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
> -		set_bit(GPIOD_FLAG_IS_OUT, &flags);
> -	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
> -		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
> -

[Severity: High]
By leaving GPIOD_FLAG_IS_OUT unset when a driver refuses the output direction,
does this create a memory leak in gpiod_set_array_value_complex() when
user-space subsequently calls GPIO_V2_LINE_SET_VALUES_IOCTL?

Looking at gpiod_set_array_value_complex() in drivers/gpio/gpiolib.c, mask
and bits can be dynamically allocated with bitmap_alloc() for large GPIO
chips:

			if (unlikely(!test_bit(GPIOD_FLAG_IS_OUT, &desc->flags)))
				return -EPERM;

If a descriptor is not configured as an output, the function immediately
returns -EPERM. 

Doesn't this bypass the bitmap_free() calls at the end of the function and
leak mask and bits?

The same direct return leak appears to occur if the driver's set_multiple
callback fails:

		if (count != 0) {
			ret = gpiochip_set_multiple(guard.gc, mask, bits);
			if (ret)
				return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-gpiolib-cdev-direction-v1-1-aambrozewicz@nvidia.com?part=1