| Message ID | 20220306193420.99714-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: check for overflow when reading the 'ngpios' property | expand |
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.
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);
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);
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 > >
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.
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; }
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(+)