[v1] gpiolib: fix invalid pointer access in debugfs

[PATCH] gpiolib: fix invalid pointer access in debugfs

Posted by Bartosz Golaszewski 3 months, 1 week ago

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

If the memory allocation in gpiolib_seq_start() fails, the s->private
field remains uninitialized and is later dereferenced without checking
in gpiolib_seq_stop(). Initialize s->private to NULL before calling
kzalloc() and check it before dereferencing it.

Fixes: e348544f7994 ("gpio: protect the list of GPIO devices with SRCU")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
This was brought to my attention by a person under sanctions. This is a
simplified version of their initial patch.

 drivers/gpio/gpiolib.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a81981336b36..fdb6a002dbda 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -5303,6 +5303,8 @@ static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
 	struct gpio_device *gdev;
 	loff_t index = *pos;
 
+	s->private = NULL;
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return NULL;
@@ -5338,6 +5340,9 @@ static void gpiolib_seq_stop(struct seq_file *s, void *v)
 {
 	struct gpiolib_seq_priv *priv = s->private;
 
+	if (!priv)
+		return;
+
 	srcu_read_unlock(&gpio_devices_srcu, priv->idx);
 	kfree(priv);
 }
-- 
2.48.1

Re: [PATCH] gpiolib: fix invalid pointer access in debugfs

Posted by Andy Shevchenko 3 months ago

On Fri, Oct 31, 2025 at 04:06:31PM +0100, Bartosz Golaszewski wrote:
> 
> If the memory allocation in gpiolib_seq_start() fails, the s->private
> field remains uninitialized and is later dereferenced without checking
> in gpiolib_seq_stop(). Initialize s->private to NULL before calling
> kzalloc() and check it before dereferencing it.

...

> static void gpiolib_seq_stop(struct seq_file *s, void *v)
>  {
>  	struct gpiolib_seq_priv *priv = s->private;
>  
> +	if (!priv)
> +		return;

My preference is to have the assignment be decoupled in such a case:

	struct gpiolib_seq_priv *priv;

	priv = s->private;
	if (!priv)
		return;

This will prevent from doing subtle mistakes (as dereferencing before check and
so on) in the future. Not that I expect this function to grow that way, but still...
always keep in mind that somebody who is not familiar with the code may take the
piece as high standard in the kernel and copy to their code without much thinking.

>  	srcu_read_unlock(&gpio_devices_srcu, priv->idx);
>  	kfree(priv);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: fix invalid pointer access in debugfs

Posted by Markus Elfring 3 months, 1 week ago

…
> ---
> This was brought to my attention by a person under sanctions. This is a
> simplified version of their initial patch.
> 
>  drivers/gpio/gpiolib.c | 5 +++++
…

Would the tag “Reported-by” become relevant here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.18-rc3#n539

Regards,
Markus

Re: [PATCH] gpiolib: fix invalid pointer access in debugfs

Posted by Linus Walleij 3 months, 1 week ago

On Fri, Oct 31, 2025 at 4:06 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> If the memory allocation in gpiolib_seq_start() fails, the s->private
> field remains uninitialized and is later dereferenced without checking
> in gpiolib_seq_stop(). Initialize s->private to NULL before calling
> kzalloc() and check it before dereferencing it.
>
> Fixes: e348544f7994 ("gpio: protect the list of GPIO devices with SRCU")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpiolib: fix invalid pointer access in debugfs

Posted by Ilya Gavrilov 3 months, 1 week ago

Hi Bartosz,

On 10/31/25 18:06, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> If the memory allocation in gpiolib_seq_start() fails, the s->private
> field remains uninitialized and is later dereferenced without checking
> in gpiolib_seq_stop(). Initialize s->private to NULL before calling
> kzalloc() and check it before dereferencing it.
> 
> Fixes: e348544f7994 ("gpio: protect the list of GPIO devices with SRCU")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> This was brought to my attention by a person under sanctions. This is a
> simplified version of their initial patch.
> 
>  drivers/gpio/gpiolib.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a81981336b36..fdb6a002dbda 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -5303,6 +5303,8 @@ static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
>  	struct gpio_device *gdev;
>  	loff_t index = *pos;
>  
> +	s->private = NULL;
> +
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return NULL;
> @@ -5338,6 +5340,9 @@ static void gpiolib_seq_stop(struct seq_file *s, void *v)
>  {
>  	struct gpiolib_seq_priv *priv = s->private;
>  
> +	if (!priv)
> +		return;
> +
>  	srcu_read_unlock(&gpio_devices_srcu, priv->idx);
>  	kfree(priv);
>  }

Maybe in this case it is better to signal an error and
explicitly return ERR_PTR(-ENOMEM) in gpiolib_seq_start(), rather than ignore it?