[PATCH] gpio-sim: fix memory corruption when adding named lines and unnamed hogs

[PATCH] gpio-sim: fix memory corruption when adding named lines and unnamed hogs

[Kent Gibson]

When constructing the sim, gpio-sim constructs an array of named lines,
sized based on the largest offset of any named line, and then initializes
that array with the names of all lines, including unnamed hogs with higher
offsets.  In doing so it writes NULLs beyond the extent of the array.

Add a check that only named lines are used to initialize the array.

Fixes: cb8c474e79be ("gpio: sim: new testing module")
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

After writing the comment above, and looking at the code again, it may be
clearer to instead check that the offset is within the bounds of the
array.  Or do both.  Consider that my review.

Cheers,
Kent.

 drivers/gpio/gpio-sim.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index e5dfd636c63c..923e8ff53128 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -721,8 +721,10 @@ static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
 	if (!line_names)
 		return ERR_PTR(-ENOMEM);
 
-	list_for_each_entry(line, &bank->line_list, siblings)
-		line_names[line->offset] = line->name;
+	list_for_each_entry(line, &bank->line_list, siblings) {
+		if (line->name)
+			line_names[line->offset] = line->name;
+	}
 
 	return line_names;
 }
-- 
2.40.1

Re: [PATCH] gpio-sim: fix memory corruption when adding named lines and unnamed hogs

[Bartosz Golaszewski]

On Tue, Jun 6, 2023 at 7:13 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> When constructing the sim, gpio-sim constructs an array of named lines,
> sized based on the largest offset of any named line, and then initializes
> that array with the names of all lines, including unnamed hogs with higher
> offsets.  In doing so it writes NULLs beyond the extent of the array.
>
> Add a check that only named lines are used to initialize the array.
>
> Fixes: cb8c474e79be ("gpio: sim: new testing module")
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>
> After writing the comment above, and looking at the code again, it may be
> clearer to instead check that the offset is within the bounds of the
> array.  Or do both.  Consider that my review.
>

Like:

if (line->offset <= max_offset)
    line_names[line->offset] = line->name;

? If so, then I agree it makes the purpose of the check clearer.

Bart

> Cheers,
> Kent.
>
>  drivers/gpio/gpio-sim.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index e5dfd636c63c..923e8ff53128 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -721,8 +721,10 @@ static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
>         if (!line_names)
>                 return ERR_PTR(-ENOMEM);
>
> -       list_for_each_entry(line, &bank->line_list, siblings)
> -               line_names[line->offset] = line->name;
> +       list_for_each_entry(line, &bank->line_list, siblings) {
> +               if (line->name)
> +                       line_names[line->offset] = line->name;
> +       }
>
>         return line_names;
>  }
> --
> 2.40.1
>

Re: [PATCH] gpio-sim: fix memory corruption when adding named lines and unnamed hogs

[Kent Gibson]

On Tue, Jun 06, 2023 at 12:01:53PM +0200, Bartosz Golaszewski wrote:
> On Tue, Jun 6, 2023 at 7:13 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > When constructing the sim, gpio-sim constructs an array of named lines,
> > sized based on the largest offset of any named line, and then initializes
> > that array with the names of all lines, including unnamed hogs with higher
> > offsets.  In doing so it writes NULLs beyond the extent of the array.
> >
> > Add a check that only named lines are used to initialize the array.
> >
> > Fixes: cb8c474e79be ("gpio: sim: new testing module")
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >
> > After writing the comment above, and looking at the code again, it may be
> > clearer to instead check that the offset is within the bounds of the
> > array.  Or do both.  Consider that my review.
> >
> 
> Like:
> 
> if (line->offset <= max_offset)
>     line_names[line->offset] = line->name;
> 
> ? If so, then I agree it makes the purpose of the check clearer.
> 

Using line_names_size might be even clearer.

So, either that or 

if (line->name && (line->offset <= max_offset))
    line_names[line->offset] = line->name;

to also not repeat the zeroing that the kcalloc() did.

Too many options.  Let me know which you prefer.

Cheers,
Kent.

Re: [PATCH] gpio-sim: fix memory corruption when adding named lines and unnamed hogs

[Bartosz Golaszewski]

On Tue, Jun 6, 2023 at 12:19 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Jun 06, 2023 at 12:01:53PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Jun 6, 2023 at 7:13 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > When constructing the sim, gpio-sim constructs an array of named lines,
> > > sized based on the largest offset of any named line, and then initializes
> > > that array with the names of all lines, including unnamed hogs with higher
> > > offsets.  In doing so it writes NULLs beyond the extent of the array.
> > >
> > > Add a check that only named lines are used to initialize the array.
> > >
> > > Fixes: cb8c474e79be ("gpio: sim: new testing module")
> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > ---
> > >
> > > After writing the comment above, and looking at the code again, it may be
> > > clearer to instead check that the offset is within the bounds of the
> > > array.  Or do both.  Consider that my review.
> > >
> >
> > Like:
> >
> > if (line->offset <= max_offset)
> >     line_names[line->offset] = line->name;
> >
> > ? If so, then I agree it makes the purpose of the check clearer.
> >
>
> Using line_names_size might be even clearer.
>
> So, either that or
>
> if (line->name && (line->offset <= max_offset))
>     line_names[line->offset] = line->name;
>

Yeah, let's go with this one.

Bart

> to also not repeat the zeroing that the kcalloc() did.
>
> Too many options.  Let me know which you prefer.
>
> Cheers,
> Kent.
>
>
>