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: <20241025153424.GA182871@rigel>
Date: Fri, 25 Oct 2024 23:34:24 +0800
From: Kent Gibson <warthog618@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 1/5] gpio: sysfs: use cleanup guards for
 gpiod_data::mutex

On Fri, Oct 25, 2024 at 04:08:00PM +0200, Bartosz Golaszewski wrote:
> On Fri, Oct 25, 2024 at 3:24 PM Kent Gibson <warthog618@...il.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 02:18:51PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > >
> > > Shrink the code and drop some goto labels by using lock guards around
> > > gpiod_data::mutex.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > > ---
> > >  drivers/gpio/gpiolib-sysfs.c | 81 ++++++++++++++++----------------------------
> > >  1 file changed, 29 insertions(+), 52 deletions(-)
> > >
> > > @@ -139,19 +132,17 @@ static ssize_t value_store(struct device *dev,
> > >       long value;
> > >
> > >       status = kstrtol(buf, 0, &value);
> > > +     if (status)
> > > +             return status;
> > >
> > > -     mutex_lock(&data->mutex);
> > > +     guard(mutex)(&data->mutex);
> > >
> > > -     if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
> > > -             status = -EPERM;
> > > -     } else if (status == 0) {
> > > -             gpiod_set_value_cansleep(desc, value);
> > > -             status = size;
> > > -     }
> > > +     if (!test_bit(FLAG_IS_OUT, &desc->flags))
> > > +             return -EPERM;
> > >
> > > -     mutex_unlock(&data->mutex);
> > > +     gpiod_set_value_cansleep(desc, value);
> > >
> > > -     return status;
> > > +     return size;
> > >  }
> >
> > This is a behavioural change as you've moved the decode check before the
> > permission check.  Not sure if that is significant or not, so in my
> > suggestion I retained the old order.
> >
> > Cheers,
> > Kent.
>
> Yeah, I don't know why it was done. Typically you want to sanitize the
> input before anything else and this is what's done almost everywhere
> else. I'd keep it like that.

Not knowing why it was done was precisely the reason I thought it
should be left as is.  The fact that the checks are performed in the
other order elsewhere makes me think this one was done intentionally.
Conceivably it could be used by userspace to test if a line is output when
the direction is fixed (so /sys/class/gpio/gpioN/direction does not exist).
So write a non-integer to the value and see if it returns -EPERM rather
than -EINVAL.

Admittedly I'm speculating, but I can't rule it out, so I wouldn't
change the behaviour just because it is more aesthetically pleasing.
And if you insist on tidying the behaviour then it should be in a separate
patch rather than piggy-backing onto the guard change.

Anyway, that is my 2c.

Cheers,
Kent.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ