From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

In `gpiod_get_label()`, it is possible that `srcu_dereference_check()` may
return a NULL pointer, leading to a scenario where `label->str` is accessed
without verifying if `label` itself is NULL.

This patch adds a proper NULL check for `label` before accessing
`label->str`. The check for `label->str != NULL` is removed because
`label->str` can never be NULL if `label` is not NULL.

This fixes the issue where the label name was being printed as `(efault)`
when dumping the sysfs GPIO file when `label == NULL`.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Output before this patch:
root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio | grep P2_1
 gpio-529 (P2_1                |(efault)            ) in  hi IRQ

Output after this patch:
root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio | grep P2_1
 gpio-529 (P2_1                |interrupt           ) in  hi IRQ
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c6afbf434366..d5952ab7752c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -114,12 +114,12 @@ const char *gpiod_get_label(struct gpio_desc *desc)
 				srcu_read_lock_held(&desc->gdev->desc_srcu));
 
 	if (test_bit(FLAG_USED_AS_IRQ, &flags))
-		return label->str ?: "interrupt";
+		return label ? label->str : "interrupt";
 
 	if (!test_bit(FLAG_REQUESTED, &flags))
 		return NULL;
 
-	return label->str;
+	return label ? label->str : NULL;
 }
 
 static void desc_free_label(struct rcu_head *rh)
-- 
2.43.0

On Thu, Oct 3, 2024 at 3:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> In `gpiod_get_label()`, it is possible that `srcu_dereference_check()` may
> return a NULL pointer, leading to a scenario where `label->str` is accessed
> without verifying if `label` itself is NULL.
>
> This patch adds a proper NULL check for `label` before accessing
> `label->str`. The check for `label->str != NULL` is removed because
> `label->str` can never be NULL if `label` is not NULL.
>
> This fixes the issue where the label name was being printed as `(efault)`
> when dumping the sysfs GPIO file when `label == NULL`.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Neat!

Took me an aha-erlebnis to realize why it didn't crash immediately when
accessing label->str, but it just returns the address relative to the
zero base...

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

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

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


On Thu, 03 Oct 2024 14:13:51 +0100, Prabhakar wrote:
> In `gpiod_get_label()`, it is possible that `srcu_dereference_check()` may
> return a NULL pointer, leading to a scenario where `label->str` is accessed
> without verifying if `label` itself is NULL.
> 
> This patch adds a proper NULL check for `label` before accessing
> `label->str`. The check for `label->str != NULL` is removed because
> `label->str` can never be NULL if `label` is not NULL.
> 
> [...]

Good catch, thanks! I added Fixes: tags and queued it for rc2.

[1/1] gpiolib: Fix potential NULL pointer dereference in gpiod_get_label()
      commit: 7b99b5ab885993bff010ebcd93be5e511c56e28a

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