Skip to contentSkip to conversation

gpiolib: fix invalid pointer access in debugfs

PATCH Superseded Bartosz Golaszewski 5 participants 5 messages Oct 31, 2025 – Nov 3, 2025 GPIO

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

PATCH 1/1 gpiolib: fix invalid pointer access in debugfs

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
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(+)
drivers/gpio/gpiolib.c +5 -0
index a81981336b36..fdb6a002dbda 100644
@@ -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);
 }
Signature · 1 line
2.48.1
#1 lore raw

Hi Bartosz,

Quoted 30 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
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(+)
drivers/gpio/gpiolib.c +2 -0
index a81981336b36..fdb6a002dbda 100644
@@ -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;

drivers/gpio/gpiolib.c static void gpiolib_seq_stop(struct seq_file *s, void *v) · line 5348

Quoted 9 lines
drivers/gpio/gpiolib.c +3 -0
@@ -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?

Reviewed-by on v1 1/1
#2 lore raw
In reply to Bartosz Golaszewski
Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

#4 lore raw
In reply to Bartosz Golaszewski
Quoted 5 lines · Bartosz Golaszewski

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.

...

drivers/gpio/gpiolib.c static void gpiolib_seq_stop(struct seq_file *s, void *v) · line 5344

Quoted 6 lines

static void gpiolib_seq_stop(struct seq_file *s, void *v)
{

drivers/gpio/gpiolib.c +2 -0
 	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.

Quoted 3 lines
drivers/gpio/gpiolib.c +0 -0
 	srcu_read_unlock(&gpio_devices_srcu, priv->idx);
 	kfree(priv);
 }
Signature · 2 lines
With Best Regards,
Andy Shevchenko