gpio: cdev: sanitize the label before requesting the interrupt

From: Bartosz Golaszewski <[email protected]>

When an interrupt is requested, a procfs directory is created under "/proc/irq/<irqnum>/<label>" where <label> is the string passed to one of the request_irq() variants.

What follows is that the string must not contain the "/" character or the procfs mkdir operation will fail. We don't have such constraints for GPIO consumer labels which are used verbatim as interrupt labels for GPIO irqs. We must therefore sanitize the consumer string before requesting the interrupt.

Let's replace all "/" with "-".

Cc [email protected]

Reported-by Stefan Wahren <[email protected]>

Closes https://lore.kernel.org/linux-gpio/[email protected]/ open here

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib-cdev.c +22 -4

index f384fa278764..8b5e8e92cbb5 100644
@@ -1083,10 +1083,20 @@ 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)
+{
+	return kstrdup_and_replace(orig, '/', '-', GFP_KERNEL);
+}
+
+static inline void free_irq_label(const char *label)
+{
+	kfree(label);
+}
+
 static void edge_detector_stop(struct line *line)
 {
 	if (line->irq) {
-		free_irq(line->irq, line);
+		free_irq_label(free_irq(line->irq, line));
 		line->irq = 0;
 	}
 

@@ -1110,6 +1120,7 @@ static int edge_detector_setup(struct line *line,
 	unsigned long irqflags = 0;
 	u64 eflags;
 	int irq, ret;
+	char *label;
 
 	eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS;
 	if (eflags && !kfifo_initialized(&line->req->events)) {

@@ -1146,11 +1157,17 @@ static int edge_detector_setup(struct line *line,
 			IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
 	irqflags |= IRQF_ONESHOT;
 
+	label = make_irq_label(line->req->label);
+	if (!label)
+		return -ENOMEM;
+
 	/* Request a thread to read the events */
 	ret = request_threaded_irq(irq, edge_irq_handler, edge_irq_thread,
-				   irqflags, line->req->label, line);
-	if (ret)
+				   irqflags, label, line);
+	if (ret) {
+		free_irq_label(label);
 		return ret;
+	}
 
 	line->irq = irq;
 	return 0;

@@ -1973,7 +1990,7 @@ static void lineevent_free(struct lineevent_state *le)
 		blocking_notifier_chain_unregister(&le->gdev->device_notifier,
 						   &le->device_unregistered_nb);
 	if (le->irq)
-		free_irq(le->irq, le);
+		free_irq_label(free_irq(le->irq, le));
 	if (le->desc)
 		gpiod_free(le->desc);
 	kfree(le->label);

@@ -2114,6 +2131,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	int fd;
 	int ret;
 	int irq, irqflags = 0;
+	char *label;
 
 	if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
 		return -EFAULT;

drivers/gpio/gpiolib-cdev.c +1 -2

 	/* Request a thread to read the events */
 	ret = request_threaded_irq(irq,
 				   lineevent_irq_handler,
 				   lineevent_irq_thread,
 				   irqflags,
-				   le->label,
+				   label,
 				   le);
 	if (ret)
 		goto out_free_le;
--

2.40.1

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 2 lines

Let's replace all "/" with "-".

I actually prefer the ":" you originally suggested, as it more clearly indicates a tier separation, whereas a hyphen is commonly used for multi-word names. And as the hyphen is more commonly used the sanitized name is more likely to conflict.

drivers/gpio/gpiolib-cdev.c static int lineevent_create(struct gpio_device *gdev, void __user *ip) · line 2222

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+     label = make_irq_label(le->label);
+     if (!label)
+             goto out_free_le;
+

Need to set ret = -ENOMEM before the goto, else you will return 0.

Quoted 4 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 8 lines

Quoted 2 lines

Let's replace all "/" with "-".

I actually prefer the ":" you originally suggested, as it more clearly indicates a tier separation, whereas a hyphen is commonly used for multi-word names. And as the hyphen is more commonly used the sanitized name is more likely to conflict.

Sounds good, will do.

Quoted 6 lines

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+     label = make_irq_label(le->label);
+     if (!label)
+             goto out_free_le;
+

Quoted 2 lines

Need to set ret = -ENOMEM before the goto, else you will return 0.

Eek, right, thanks.

Quoted 21 lines · Bartosz Golaszewski

Quoted 4 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 8 lines

Quoted 2 lines

Let's replace all "/" with "-".

I actually prefer the ":" you originally suggested, as it more clearly indicates a tier separation, whereas a hyphen is commonly used for multi-word names. And as the hyphen is more commonly used the sanitized name is more likely to conflict.

Sounds good, will do.

Quoted 6 lines

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+     label = make_irq_label(le->label);
+     if (!label)
+             goto out_free_le;
+

Quoted 4 lines

Quoted 2 lines

Need to set ret = -ENOMEM before the goto, else you will return 0.

Eek, right, thanks.

Smatch has a warning about this, btw. drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret'

The other warning here is:
drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258.

Quoted 22 lines · Dan Carpenter

Quoted 21 lines · Bartosz Golaszewski

Quoted 4 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 8 lines

Quoted 2 lines

Let's replace all "/" with "-".

I actually prefer the ":" you originally suggested, as it more clearly indicates a tier separation, whereas a hyphen is commonly used for multi-word names. And as the hyphen is more commonly used the sanitized name is more likely to conflict.

Sounds good, will do.

Quoted 6 lines

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+     label = make_irq_label(le->label);
+     if (!label)
+             goto out_free_le;
+

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Need to set ret = -ENOMEM before the goto, else you will return 0.

Eek, right, thanks.

Smatch has a warning about this, btw. drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret'

And that triggered a "what the hell does that mean" warning in my wetware error parser ;-).

That could be better worded - it isn't "missing", it hasn't been appropriately set. So maybe "unset error code"?

Quoted 3 lines

The other warning here is:
drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258.

Looks like a false positive to me - as per the comment in the code, that path (2258) results in lineevent_release() being called and that releases the irq.