linux-kernel - Re: [PATCH v2 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 19 Nov 2021 20:35:33 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] gpiolib: check the 'ngpios' property in core
 gpiolib code

On Thu, Nov 18, 2021 at 10:16 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Thu, Nov 18, 2021 at 09:12:59PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Nov 18, 2021 at 6:06 PM Andy Shevchenko
> > <andriy.shevchenko@...ux.intel.com> wrote:
> > > On Thu, Nov 18, 2021 at 02:23:17PM +0100, Bartosz Golaszewski wrote:
> > > > Several drivers read the 'ngpios' device property on their own, but
> > > > since it's defined as a standard GPIO property in the device tree bindings
> > > > anyway, it's a good candidate for generalization. If the driver didn't
> > > > set its gc->ngpio, try to read the 'ngpios' property from the GPIO
> > > > device's firmware node before bailing out.
> > >
> > > ...
> > >
> > > >       if (gc->ngpio == 0) {
> > > > -             chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > > > -             ret = -EINVAL;
> > > > -             goto err_free_descs;
> > > > +             ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
> > > > +             if (ret) {
> > > > +                     chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > > > +                     ret = -EINVAL;
> > > > +                     goto err_free_descs;
> > > > +             }
> > > > +
> > > > +             gc->ngpio = ngpios;
> > > >       }
> > >
> > > This should be
> > >
> > >         if (gc->ngpio == 0) {
> > >                 ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
> > >                 if (ret)
> > >                         return ret;
> >
> > But device_property_read_u32() returning -ENODATA means there's no
> > such property, which should actually be converted to -EINVAL as the
> > caller wanting to create the chip provided invalid configuration - in
> > this case: a chip with 0 lines. In case of the non-array variant of
> > read_u32 that's also the only error that can be returned so this bit
> > looks right to me.
>
> So, what is so special about -EINVAL? Why -ENODATA is not good enough which
> will exactly explain to the caller what's going on, no?
>

Let's imagine the user sets gc->ngpio = 0 incorrectly thinking it'll
make gpiolib set it to some sane default. Then gpiochip_add_data()
returns -ENODATA (No data available). This is confusing IMO. But if we
convert it to -EINVAL, it now says "Invalid value" which points to the
wrong configuration.

ENODATA means "device tree property is not present" in this case but
the problem is that user supplies the gpiolib with invalid
configuration. EINVAL is the right error here.

Bart

> > >                 gc->ngpio = ngpios;
> > >         }
> > >
> > >         if (gc->ngpio == 0) {
> > >                 chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > >                 ret = -EINVAL;
> > >                 goto err_free_descs;
>
> When the caller intended to create a chip with 0 GPIOs they will get an error
> as you wish with an error message.
>

Yes, as it was before.

Bart

> > >         }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>