linux-kernel - Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 27 Jun 2024 16:06:21 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Kent Gibson <warthog618@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, 
	linus.walleij@...aro.org
Subject: Re: [PATCH 2/4] gpiolib: cdev: Ignore reconfiguration without direction

On Wed, Jun 26, 2024 at 7:29 AM Kent Gibson <warthog618@...il.com> wrote:
>
> linereq_set_config() behaves badly when direction is not set.
> The configuration validation is borrowed from linereq_create(), where,
> to verify the intent of the user, the direction must be set to in order to
> effect a change to the electrical configuration of a line. But, when
> applied to reconfiguration, that validation does not allow for the unset
> direction case, making it possible to clear flags set previously without
> specifying the line direction.
>
> Adding to the inconsistency, those changes are not immediately applied by
> linereq_set_config(), but will take effect when the line value is next get
> or set.
>
> For example, by requesting a configuration with no flags set, an output
> line with GPIO_V2_LINE_FLAG_ACTIVE_LOW and GPIO_V2_LINE_FLAG_OPEN_DRAIN
> set could have those flags cleared, inverting the sense of the line and
> changing the line drive to push-pull on the next line value set.
>
> Skip the reconfiguration of lines for which the direction is not set, and
> only reconfigure the lines for which direction is set.
>
> Fixes: a54756cb24ea ("gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL")
> Signed-off-by: Kent Gibson <warthog618@...il.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index f7a129d67b7d..ef08b23a56e2 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1534,12 +1534,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>                 line = &lr->lines[i];
>                 desc = lr->lines[i].desc;
>                 flags = gpio_v2_line_config_flags(&lc, i);
> +               /*
> +                * Lines not explicitly reconfigured as input or output
> +                * are left unchanged.
> +                */
> +               if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> +                       continue;

Series looks good, thanks. I'd say that this bit here calls for at
least a debug-level message since we don't return an error unlike v1.
What do you think?

Bart

>                 gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
>                 edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
> -               /*
> -                * Lines have to be requested explicitly for input
> -                * or output, else the line will be treated "as is".
> -                */
>                 if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
>                         int val = gpio_v2_line_config_output_value(&lc, i);
>
> @@ -1547,7 +1549,7 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>                         ret = gpiod_direction_output(desc, val);
>                         if (ret)
>                                 return ret;
> -               } else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
> +               } else {
>                         ret = gpiod_direction_input(desc);
>                         if (ret)
>                                 return ret;
> --
> 2.39.2
>