The "> chip->ngpio" comparison here needs to be ">= chip->ngpio",
otherwise it leads to an out of bounds access.  The fwd->valid_mask
bitmap only has chip->ngpio bits and the fwd->descs[] array has that
same number of elements.  These values are set in
devm_gpiochip_fwd_alloc().

Fixes: c44ce91b8ada ("gpio: aggregator: refactor the code to add GPIO desc in the forwarder")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/gpio/gpio-aggregator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 0ef6556f98b1..37600faf4a4b 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -744,7 +744,7 @@ int gpiochip_fwd_desc_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 {
 	struct gpio_chip *chip = &fwd->chip;
 
-	if (offset > chip->ngpio)
+	if (offset >= chip->ngpio)
 		return -EINVAL;
 
 	if (test_and_set_bit(offset, fwd->valid_mask))
-- 
2.47.2

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 13 Aug 2025 08:38:27 +0300, Dan Carpenter wrote:
> The "> chip->ngpio" comparison here needs to be ">= chip->ngpio",
> otherwise it leads to an out of bounds access.  The fwd->valid_mask
> bitmap only has chip->ngpio bits and the fwd->descs[] array has that
> same number of elements.  These values are set in
> devm_gpiochip_fwd_alloc().
> 
> 
> [...]

Applied, thanks!

[1/1] gpio: aggregator: Fix off by one in gpiochip_fwd_desc_add()
      https://git.kernel.org/brgl/linux/c/148547000cfc1ba8cec02857268333d08724b9cc

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

On Wed, Aug 13, 2025 at 9:23 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> On Wed, 13 Aug 2025 08:38:27 +0300, Dan Carpenter wrote:
> > The "> chip->ngpio" comparison here needs to be ">= chip->ngpio",
> > otherwise it leads to an out of bounds access.  The fwd->valid_mask
> > bitmap only has chip->ngpio bits and the fwd->descs[] array has that
> > same number of elements.  These values are set in
> > devm_gpiochip_fwd_alloc().
> >
> >
> > [...]
>
> Applied, thanks!
>
> [1/1] gpio: aggregator: Fix off by one in gpiochip_fwd_desc_add()
>       https://git.kernel.org/brgl/linux/c/148547000cfc1ba8cec02857268333d08724b9cc

Do I need this for the aggregator immutable branch I merged yesterday?

I have only merged that branch to my new development series, if
you need me to pull in a new version just send a new pull request
and I will use that instead.

Yours,
Linus Walleij

On Thu, 14 Aug 2025 at 10:42, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Aug 13, 2025 at 9:23 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > On Wed, 13 Aug 2025 08:38:27 +0300, Dan Carpenter wrote:
> > > The "> chip->ngpio" comparison here needs to be ">= chip->ngpio",
> > > otherwise it leads to an out of bounds access.  The fwd->valid_mask
> > > bitmap only has chip->ngpio bits and the fwd->descs[] array has that
> > > same number of elements.  These values are set in
> > > devm_gpiochip_fwd_alloc().
> > >
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [1/1] gpio: aggregator: Fix off by one in gpiochip_fwd_desc_add()
> >       https://git.kernel.org/brgl/linux/c/148547000cfc1ba8cec02857268333d08724b9cc
>
> Do I need this for the aggregator immutable branch I merged yesterday?
>
> I have only merged that branch to my new development series, if
> you need me to pull in a new version just send a new pull request
> and I will use that instead.
>
> Yours,
> Linus Walleij

No, it's not a build-time dependency and it will end up in next anyway
from my tree.

Bart

On Wed, 13 Aug 2025 at 07:38, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> The "> chip->ngpio" comparison here needs to be ">= chip->ngpio",
> otherwise it leads to an out of bounds access.  The fwd->valid_mask
> bitmap only has chip->ngpio bits and the fwd->descs[] array has that
> same number of elements.  These values are set in
> devm_gpiochip_fwd_alloc().
>
> Fixes: c44ce91b8ada ("gpio: aggregator: refactor the code to add GPIO desc in the forwarder")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -744,7 +744,7 @@ int gpiochip_fwd_desc_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
>  {
>         struct gpio_chip *chip = &fwd->chip;
>
> -       if (offset > chip->ngpio)
> +       if (offset >= chip->ngpio)
>                 return -EINVAL;
>
>         if (test_and_set_bit(offset, fwd->valid_mask))

Looks like my similar comment in
https://lore.kernel.org/all/CAMuHMdVLo2w609eFOKRkYAfEMb8XOTNB-XzzZn_89VM-YV_-kA@mail.gmail.com/
was lost in the noise. I'll try to remember to make ">=" stand out more
among all quoted code.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds