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

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

[Adrian Ambrozewicz]

When a line is requested or reconfigured through the v2 uAPI,
gpio_v2_line_config_flags_to_desc_flags() copies the requested
configuration into the descriptor flags, GPIOD_FLAG_IS_OUT included,
before gpiod_direction_input_nonotify() or
gpiod_direction_output_nonotify() asks the driver for the direction.
If the driver refuses, for instance with -EOPNOTSUPP from
.direction_input() for a line that can only be an output, the ioctl
fails but the flag keeps the refused direction. gpiod_free_commit()
does not touch GPIOD_FLAG_IS_OUT, so this outlives the failed request,
and GPIO_V2_GET_LINEINFO_IOCTL reports the line as an input while the
hardware still drives it, until a request without direction flags
makes gpiod_request_commit() read the direction back from the driver:

  GPIO_V2_GET_LINE_IOCTL, line 0, input:         -EOPNOTSUPP
  GPIO_V2_GET_LINEINFO_IOCTL, line 0:            input
  GPIO_V2_GET_LINE_IOCTL, line 0, no direction:  ok
  GPIO_V2_GET_LINEINFO_IOCTL, line 0:            output

GPIO_V2_LINE_SET_CONFIG_IOCTL updates the flags in the same order.
There the line stays requested, and since commit 92ac7de3175e
("gpiolib: don't allow setting values on input lines")
GPIO_V2_LINE_SET_VALUES_IOCTL on it then fails with -EPERM, though the
line is still an output. A line whose driver refuses to make it an
output is likewise reported as one.

gpiod_direction_input_nonotify() and gpiod_direction_output_nonotify()
set or clear GPIOD_FLAG_IS_OUT themselves once the driver has accepted
the direction, emulated open drain and open source included, and
nothing between the copy and those calls reads the flag. Leave it to
them, as the v1 uAPI already does.

The other flags that the helper copies stay as they are. Active low,
drive and bias are what gpiod_direction_*() applies, drive and bias
possibly before the driver refuses the direction; edges and event
clock are the character device's own. None of them is read back from
the driver, so there is no earlier state that is more accurate to go
back to. A failed request releases the line, which clears them, and a
failed reconfiguration leaves the line, like the lines before it, with
what was applied up to the failure.

Reproduced under QEMU with a test GPIO chip whose line 0 refuses input
and line 1 refuses output: both refused requests and both refused
reconfigurations leave the line info with the refused direction, and
setting the value of the reconfigured output fails with -EPERM. With
this change the line info shows the direction of the hardware in all
four cases and setting the value works. First seen on real hardware
with v6.12.

Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
 drivers/gpio/gpiolib-cdev.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5d53bfcdf726..9bbee3a1f9c9 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);
 
+	/*
+	 * 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);
-
 	assign_bit(GPIOD_FLAG_EDGE_RISING, &flags,
 		   lflags & GPIO_V2_LINE_FLAG_EDGE_RISING);
 	assign_bit(GPIOD_FLAG_EDGE_FALLING, &flags,

base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.43.0

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

[sashiko-bot]

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