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]

Message-ID: <CAMRc=Md9eGjfytYbnMunbda_wQiaon2AwbGrRMW4WP8NhF6z4Q@mail.gmail.com>
Date: Thu, 27 Jun 2024 16:51:13 +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 Thu, Jun 27, 2024 at 4:49 PM Kent Gibson <warthog618@...il.com> wrote:
>
> On Thu, Jun 27, 2024 at 04:44:02PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Jun 27, 2024 at 4:22 PM Kent Gibson <warthog618@...il.com> wrote:
> > >
> > > On Thu, Jun 27, 2024 at 04:06:21PM +0200, Bartosz Golaszewski wrote:
> > > > 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?
> > > >
> > >
> > > The change to the libgpiod Python bindings makes use of this to support
> > > reconfiguration of subsets, so on its own it isn't an abnormal path and
> > > I'm not sure it warrants even a debug.
> > >
> > > OTOH, I did consider if there should be a check that at least one line
> > > in the reconfig has a direction, returning an error if there are none, but
> > > was on the fence about it and left it out as it added complexity.
> > >
> > > Would that make more sense?
> > > Or do you have a problem with reconfiguring subsets?
> > >
> > > Cheers,
> > > Kent.
> >
> > I see. Ok, I'll take it as is interpreting it as a feature.
> >
>
> I'm totally ok with adding a check that direction is set at least once,
> if you would like that. Can be done with a reasonably minor change to
> gpio_v2_line_config_validate().  Though that would probably still double
> the size of this patch.
>
> Cheers,
> Kent.
>

No, it's fine. Docs are quite explicit about the behavior and there's
a comment in place.

Bart