[PATCH v2 0/2] gpio: cdev: label sanitization fixes

[PATCH v2 0/2] gpio: cdev: label sanitization fixes

[Bartosz Golaszewski]

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

This series fixes a couple of bugs in the sanitization of labels
being passed to irq.

Patch 1 fixes the case where userspace provides empty labels.

Patch 2 fixes a missed path in the sanitization changes that can result
in memory corruption.

v1 -> v2:
- switched the order of the patches in order to avoid introducing buggy
  code in one just to fix it in the second

Bartosz Golaszewski (1):
  gpio: cdev: check for NULL labels when sanitizing them for irqs

Kent Gibson (1):
  gpio: cdev: fix missed label sanitizing in debounce_setup()

 drivers/gpio/gpiolib-cdev.c | 46 +++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 15 deletions(-)

-- 
2.40.1

[PATCH v2 1/2] gpio: cdev: check for NULL labels when sanitizing them for irqs

[Bartosz Golaszewski]

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

We need to take into account that a line's consumer label may be NULL
and not try to kstrdup() it in that case but rather pass the NULL
pointer up the stack to the interrupt request function.

To that end: let make_irq_label() return NULL as a valid return value
and use ERR_PTR() instead to signal an allocation failure to callers.

Cc: stable@vger.kernel.org
Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Closes: https://lore.kernel.org/lkml/20240402093534.212283-1-naresh.kamboju@linaro.org/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index fa9635610251..1426cc1c4a28 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1085,7 +1085,16 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
 
 static inline char *make_irq_label(const char *orig)
 {
-	return kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
+	char *new;
+
+	if (!orig)
+		return NULL;
+
+	new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
+	if (!new)
+		return ERR_PTR(-ENOMEM);
+
+	return new;
 }
 
 static inline void free_irq_label(const char *label)
@@ -1158,8 +1167,8 @@ static int edge_detector_setup(struct line *line,
 	irqflags |= IRQF_ONESHOT;
 
 	label = make_irq_label(line->req->label);
-	if (!label)
-		return -ENOMEM;
+	if (IS_ERR(label))
+		return PTR_ERR(label);
 
 	/* Request a thread to read the events */
 	ret = request_threaded_irq(irq, edge_irq_handler, edge_irq_thread,
@@ -2217,8 +2226,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 		goto out_free_le;
 
 	label = make_irq_label(le->label);
-	if (!label) {
-		ret = -ENOMEM;
+	if (IS_ERR(label)) {
+		ret = PTR_ERR(label);
 		goto out_free_le;
 	}
 
-- 
2.40.1

Re: [PATCH v2 1/2] gpio: cdev: check for NULL labels when sanitizing them for irqs

[Bartosz Golaszewski]

On Thu, Apr 4, 2024 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We need to take into account that a line's consumer label may be NULL
> and not try to kstrdup() it in that case but rather pass the NULL
> pointer up the stack to the interrupt request function.
>
> To that end: let make_irq_label() return NULL as a valid return value
> and use ERR_PTR() instead to signal an allocation failure to callers.
>
> Cc: stable@vger.kernel.org
> Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Closes: https://lore.kernel.org/lkml/20240402093534.212283-1-naresh.kamboju@linaro.org/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Tested-by: Anders Roxell <anders.roxell@linaro.org>

[PATCH v2 2/2] gpio: cdev: fix missed label sanitizing in debounce_setup()

[Bartosz Golaszewski]

From: Kent Gibson <warthog618@gmail.com>

When adding sanitization of the label, the path through
edge_detector_setup() that leads to debounce_setup() was overlooked.
A request taking this path does not allocate a new label and the
request label is freed twice when the request is released, resulting
in memory corruption.

Add label sanitization to debounce_setup().

Cc: stable@vger.kernel.org
Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
Signed-off-by: Kent Gibson <warthog618@gmail.com>
[Bartosz: rebased on top of the fix for empty GPIO labels]
Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 47 +++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 1426cc1c4a28..6fe978535047 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -728,6 +728,25 @@ static u32 line_event_id(int level)
 		       GPIO_V2_LINE_EVENT_FALLING_EDGE;
 }
 
+static inline char *make_irq_label(const char *orig)
+{
+	char *new;
+
+	if (!orig)
+		return NULL;
+
+	new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
+	if (!new)
+		return ERR_PTR(-ENOMEM);
+
+	return new;
+}
+
+static inline void free_irq_label(const char *label)
+{
+	kfree(label);
+}
+
 #ifdef CONFIG_HTE
 
 static enum hte_return process_hw_ts_thread(void *p)
@@ -1015,6 +1034,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 {
 	unsigned long irqflags;
 	int ret, level, irq;
+	char *label;
 
 	/* try hardware */
 	ret = gpiod_set_debounce(line->desc, debounce_period_us);
@@ -1037,11 +1057,17 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 			if (irq < 0)
 				return -ENXIO;
 
+			label = make_irq_label(line->req->label);
+			if (IS_ERR(label))
+				return -ENOMEM;
+
 			irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
 			ret = request_irq(irq, debounce_irq_handler, irqflags,
 					  line->req->label, line);
-			if (ret)
+			if (ret) {
+				free_irq_label(label);
 				return ret;
+			}
 			line->irq = irq;
 		} else {
 			ret = hte_edge_setup(line, GPIO_V2_LINE_FLAG_EDGE_BOTH);
@@ -1083,25 +1109,6 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
 	return 0;
 }
 
-static inline char *make_irq_label(const char *orig)
-{
-	char *new;
-
-	if (!orig)
-		return NULL;
-
-	new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
-	if (!new)
-		return ERR_PTR(-ENOMEM);
-
-	return new;
-}
-
-static inline void free_irq_label(const char *label)
-{
-	kfree(label);
-}
-
 static void edge_detector_stop(struct line *line)
 {
 	if (line->irq) {
-- 
2.40.1

Re: [PATCH v2 2/2] gpio: cdev: fix missed label sanitizing in debounce_setup()

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 11:33:28AM +0200, Bartosz Golaszewski wrote:
> From: Kent Gibson <warthog618@gmail.com>
> 
> When adding sanitization of the label, the path through
> edge_detector_setup() that leads to debounce_setup() was overlooked.
> A request taking this path does not allocate a new label and the
> request label is freed twice when the request is released, resulting
> in memory corruption.
> 
> Add label sanitization to debounce_setup().

...

> +static inline char *make_irq_label(const char *orig)
> +{
> +	char *new;
> +
> +	if (!orig)
> +		return NULL;
> +
> +	new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
> +	if (!new)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return new;
> +}
> +
> +static inline void free_irq_label(const char *label)
> +{
> +	kfree(label);
> +}

First of all this could have been done in the previous patch already, but okay.

...

> +			label = make_irq_label(line->req->label);
> +			if (IS_ERR(label))
> +				return -ENOMEM;
> +
>  			irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
>  			ret = request_irq(irq, debounce_irq_handler, irqflags,
>  					  line->req->label, line);

But the main point how does this change fix anything?

Shouldn't be

-					  line->req->label, line);
+					  label, line);

?

> +			if (ret) {
> +				free_irq_label(label);
>  				return ret;
> +			}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpio: cdev: fix missed label sanitizing in debounce_setup()

[Bartosz Golaszewski]

On Thu, Apr 4, 2024 at 5:36 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Apr 04, 2024 at 11:33:28AM +0200, Bartosz Golaszewski wrote:
> > From: Kent Gibson <warthog618@gmail.com>
> >
> > When adding sanitization of the label, the path through
> > edge_detector_setup() that leads to debounce_setup() was overlooked.
> > A request taking this path does not allocate a new label and the
> > request label is freed twice when the request is released, resulting
> > in memory corruption.
> >
> > Add label sanitization to debounce_setup().
>
> ...
>
> > +static inline char *make_irq_label(const char *orig)
> > +{
> > +     char *new;
> > +
> > +     if (!orig)
> > +             return NULL;
> > +
> > +     new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
> > +     if (!new)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     return new;
> > +}
> > +
> > +static inline void free_irq_label(const char *label)
> > +{
> > +     kfree(label);
> > +}
>
> First of all this could have been done in the previous patch already, but okay.
>
> ...
>
> > +                     label = make_irq_label(line->req->label);
> > +                     if (IS_ERR(label))
> > +                             return -ENOMEM;
> > +
> >                       irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
> >                       ret = request_irq(irq, debounce_irq_handler, irqflags,
> >                                         line->req->label, line);
>
> But the main point how does this change fix anything?
>
> Shouldn't be
>
> -                                         line->req->label, line);
> +                                         label, line);

It should, I badly copy-pasted Kent's correct code. Thanks, I fixed it in tree.

Bart

>
> ?
>
> > +                     if (ret) {
> > +                             free_irq_label(label);
> >                               return ret;
> > +                     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 0/2] gpio: cdev: label sanitization fixes

[Bartosz Golaszewski]

On Thu, Apr 4, 2024 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This series fixes a couple of bugs in the sanitization of labels
> being passed to irq.
>
> Patch 1 fixes the case where userspace provides empty labels.
>
> Patch 2 fixes a missed path in the sanitization changes that can result
> in memory corruption.
>
> v1 -> v2:
> - switched the order of the patches in order to avoid introducing buggy
>   code in one just to fix it in the second
>
> Bartosz Golaszewski (1):
>   gpio: cdev: check for NULL labels when sanitizing them for irqs
>
> Kent Gibson (1):
>   gpio: cdev: fix missed label sanitizing in debounce_setup()
>
>  drivers/gpio/gpiolib-cdev.c | 46 +++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
> --
> 2.40.1
>

I'll go ahead and apply it as the culprit already got upstream and
into stable branches, so let's fix it ASAP. Tomorrow once autobuilders
confirm it's fine, I'll send it to Linus.

Bart