[PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[Bartosz Golaszewski]

The ngpio fields both in struct gpio_device as well as gpio_chip are
16-bit unsigned integers. Let's not risk an overflow and check if the
property value represented as a 32-bit unsigned integer is not greater
than U16_MAX.

Fixes: 9dbd1ab20509 ("gpiolib: check the 'ngpios' property in core gpiolib code")
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a3d14277f17c..3c4f47b9ab57 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -677,6 +677,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		else if (ret)
 			goto err_free_descs;
 
+		if (ngpios > U16_MAX) {
+			ret = EINVAL;
+			goto err_free_descs;
+		}
+
 		gc->ngpio = ngpios;
 	}
 
-- 
2.30.1

Re: [PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[Andy Shevchenko]

On Mon, Mar 7, 2022 at 12:11 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> The ngpio fields both in struct gpio_device as well as gpio_chip are
> 16-bit unsigned integers. Let's not risk an overflow and check if the
> property value represented as a 32-bit unsigned integer is not greater
> than U16_MAX.

...

> +               if (ngpios > U16_MAX) {
> +                       ret = EINVAL;
> +                       goto err_free_descs;
> +               }

I don't think it's a fatal error in this case. I would perhaps print a
warning and simply use a masked (which is done implicitly by an
assignment of the different type) value. Note, the above is buggy on
the buggy DTs, where the upper part of the value is not used. After
this patch you effectively make a regression on, yes, broken DTs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[Andy Shevchenko]

On Mon, Mar 7, 2022 at 12:19 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 7, 2022 at 12:11 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > The ngpio fields both in struct gpio_device as well as gpio_chip are
> > 16-bit unsigned integers. Let's not risk an overflow and check if the
> > property value represented as a 32-bit unsigned integer is not greater
> > than U16_MAX.
>
> ...
>
> > +               if (ngpios > U16_MAX) {
> > +                       ret = EINVAL;
> > +                       goto err_free_descs;
> > +               }
>
> I don't think it's a fatal error in this case. I would perhaps print a
> warning and simply use a masked (which is done implicitly by an
> assignment of the different type) value. Note, the above is buggy on
> the buggy DTs, where the upper part of the value is not used. After
> this patch you effectively make a regression on, yes, broken DTs.

Like

    if (ngpios > U16_MAX)
        chip_warn(gc, "line cnt %u is greater than supported; use
%u\n", ngpios, (u16)ngpio);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[Andy Shevchenko]

On Mon, Mar 07, 2022 at 12:23:03AM +0200, Andy Shevchenko wrote:
> On Mon, Mar 7, 2022 at 12:19 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Mar 7, 2022 at 12:11 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > The ngpio fields both in struct gpio_device as well as gpio_chip are
> > > 16-bit unsigned integers. Let's not risk an overflow and check if the
> > > property value represented as a 32-bit unsigned integer is not greater
> > > than U16_MAX.
> >
> > ...
> >
> > > +               if (ngpios > U16_MAX) {
> > > +                       ret = EINVAL;
> > > +                       goto err_free_descs;
> > > +               }
> >
> > I don't think it's a fatal error in this case. I would perhaps print a
> > warning and simply use a masked (which is done implicitly by an
> > assignment of the different type) value. Note, the above is buggy on
> > the buggy DTs, where the upper part of the value is not used. After
> > this patch you effectively make a regression on, yes, broken DTs.
> 
> Like
> 
>     if (ngpios > U16_MAX)
>         chip_warn(gc, "line cnt %u is greater than supported; use
> %u\n", ngpios, (u16)ngpio);

Or to be on safer side move this after == 0 check as

	if (gc->ngpio != ngpios)
		chip_warn(gc, "line cnt %u is greater than supported; use %u\n", ngpios, gc->ngpio);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[Bartosz Golaszewski]

On Mon, Mar 7, 2022 at 10:53 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 07, 2022 at 12:23:03AM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 7, 2022 at 12:19 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Mon, Mar 7, 2022 at 12:11 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > The ngpio fields both in struct gpio_device as well as gpio_chip are
> > > > 16-bit unsigned integers. Let's not risk an overflow and check if the
> > > > property value represented as a 32-bit unsigned integer is not greater
> > > > than U16_MAX.
> > >
> > > ...
> > >
> > > > +               if (ngpios > U16_MAX) {
> > > > +                       ret = EINVAL;
> > > > +                       goto err_free_descs;
> > > > +               }
> > >
> > > I don't think it's a fatal error in this case. I would perhaps print a
> > > warning and simply use a masked (which is done implicitly by an
> > > assignment of the different type) value. Note, the above is buggy on
> > > the buggy DTs, where the upper part of the value is not used. After
> > > this patch you effectively make a regression on, yes, broken DTs.
> >
> > Like
> >
> >     if (ngpios > U16_MAX)
> >         chip_warn(gc, "line cnt %u is greater than supported; use
> > %u\n", ngpios, (u16)ngpio);
>
> Or to be on safer side move this after == 0 check as
>
>         if (gc->ngpio != ngpios)
>                 chip_warn(gc, "line cnt %u is greater than supported; use %u\n", ngpios, gc->ngpio);
>

ngpios is not necessarily used so this check must be in the scope of
the device property read (inside the if (gc->ngpio == 0) { block).

Bart

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

Re: [PATCH] gpiolib: check for overflow when reading the 'ngpios' property

[Andy Shevchenko]

On Mon, Mar 07, 2022 at 11:08:51AM +0100, Bartosz Golaszewski wrote:
> On Mon, Mar 7, 2022 at 10:53 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Mar 07, 2022 at 12:23:03AM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 7, 2022 at 12:19 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Mon, Mar 7, 2022 at 12:11 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > The ngpio fields both in struct gpio_device as well as gpio_chip are
> > > > > 16-bit unsigned integers. Let's not risk an overflow and check if the
> > > > > property value represented as a 32-bit unsigned integer is not greater
> > > > > than U16_MAX.
> > > >
> > > > ...
> > > >
> > > > > +               if (ngpios > U16_MAX) {
> > > > > +                       ret = EINVAL;
> > > > > +                       goto err_free_descs;
> > > > > +               }
> > > >
> > > > I don't think it's a fatal error in this case. I would perhaps print a
> > > > warning and simply use a masked (which is done implicitly by an
> > > > assignment of the different type) value. Note, the above is buggy on
> > > > the buggy DTs, where the upper part of the value is not used. After
> > > > this patch you effectively make a regression on, yes, broken DTs.
> > >
> > > Like
> > >
> > >     if (ngpios > U16_MAX)
> > >         chip_warn(gc, "line cnt %u is greater than supported; use
> > > %u\n", ngpios, (u16)ngpio);
> >
> > Or to be on safer side move this after == 0 check as
> >
> >         if (gc->ngpio != ngpios)
> >                 chip_warn(gc, "line cnt %u is greater than supported; use %u\n", ngpios, gc->ngpio);
> >
> 
> ngpios is not necessarily used so this check must be in the scope of
> the device property read (inside the if (gc->ngpio == 0) { block).

Can be done as

        if (gc->ngpio == 0) {
		...
	} else {
		ngpios = gc->ngpio;
	}

        if (gc->ngpio == 0) {
		...
	}

	if (gc->ngpio != ngpios)
		chip_warn(gc, "line cnt %u is greater than supported; use %u\n", ngpios, gc->ngpio);

The point of this exercise is to avoid hard coded type of the variable in a
few places, so if gc->ngpio and/or ngpios have changed type in the future,
you don't need to change this code.


-- 
With Best Regards,
Andy Shevchenko