Skip to contentSkip to conversation

gpioib: do not free unrequested descriptors

PATCH In review Timur Tabi 3 participants 5 messages Mar 29, 2018 – Apr 26, 2018

Subsystems (2)
Stable branch GPIO
lore
Share & tools
lore

Timur Tabi

author
#0 lore raw

PATCH 1/1 gpioib: do not free unrequested descriptors

If the main loop in linehandle_create() encounters an error, it unwinds completely by freeing all previously requested GPIO descriptors. However, if the error occurs in the beginning of the loop before that GPIO is requested, then the exit code attempts to free a null descriptor. If extrachecks is enabled, gpiod_free() triggers a WARN_ON.

Instead, keep a separate count of legitimate GPIOs so that only those are freed.

Cc [email protected]
Fixes d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Signed-off-by Timur Tabi <[email protected]>
---
 drivers/gpio/gpiolib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpio/gpiolib.c +3 -2
index 43aeb07343ec..d07771797707 100644
@@ -497,7 +497,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	struct gpiohandle_request handlereq;
 	struct linehandle_state *lh;
 	struct file *file;
-	int fd, i, ret;
+	int fd, i, count = 0, ret;
 	u32 lflags;
 
 	if (copy_from_user(&handlereq, ip, sizeof(handlereq)))

@@ -558,6 +558,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 		if (ret)
 			goto out_free_descs;
 		lh->descs[i] = desc;
+		count = i;
 
 		if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
 			set_bit(FLAG_ACTIVE_LOW, &desc->flags);

@@ -628,7 +629,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 out_put_unused_fd:
 	put_unused_fd(fd);
 out_free_descs:
-	for (; i >= 0; i--)
+	for (i = 0; i < count; i++)
 		gpiod_free(lh->descs[i]);
 	kfree(lh->label);
 out_free_lh:
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
author
#1 lore raw
Quoted 9 lines · Timur Tabi

If the main loop in linehandle_create() encounters an error, it unwinds completely by freeing all previously requested GPIO descriptors. However, if the error occurs in the beginning of the loop before that GPIO is requested, then the exit code attempts to free a null descriptor. If extrachecks is enabled, gpiod_free() triggers a WARN_ON.

Instead, keep a separate count of legitimate GPIOs so that only those are freed.

Linus, this is an important fix that's needed for sparse GPIO support. Any chance that it can make 4.17?

Also, my other patchset for qdf2xxx support has been reviewed by Bjorn and Stephen. Can you add that to 4.17 also?

Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
#2 lore raw
In reply to Timur Tabi
Quoted 7 lines · Timur Tabi

If the main loop in linehandle_create() encounters an error, it unwinds completely by freeing all previously requested GPIO descriptors. However, if the error occurs in the beginning of the loop before that GPIO is requested, then the exit code attempts to free a null descriptor. If extrachecks is enabled, gpiod_free() triggers a WARN_ON.

Quoted 6 lines

Instead, keep a separate count of legitimate GPIOs so that only those are freed.

Cc [email protected]
Fixes d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Signed-off-by Timur Tabi <[email protected]>

Patch applied for fixes.

Bartosz, can you have a quick look at this? Did you run into the problem at any point?

Yours,
Linus Walleij

#3 lore raw

2018-04-26 11:17 GMT+02:00 Linus Walleij <[email protected]>:

Quoted 12 lines · Timur Tabi
Quoted 11 lines

If the main loop in linehandle_create() encounters an error, it unwinds completely by freeing all previously requested GPIO descriptors. However, if the error occurs in the beginning of the loop before that GPIO is requested, then the exit code attempts to free a null descriptor. If extrachecks is enabled, gpiod_free() triggers a WARN_ON.

Instead, keep a separate count of legitimate GPIOs so that only those are freed.

Quoted 9 lines
Quoted 3 lines
Cc [email protected]
Fixes d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Signed-off-by Timur Tabi <[email protected]>

Patch applied for fixes.

Bartosz, can you have a quick look at this? Did you run into the problem at any point?

I have never seen this issue, but the patch looks correct to me.

Thanks,
Bartosz

author
#4 lore raw
Quoted 5 lines · Bartosz Golaszewski
Quoted 4 lines

Bartosz, can you have a quick look at this? Did you run into the problem at any point?

I have never seen this issue, but the patch looks correct to me.

The issue can only occur if you have sparse GPIOs, and you tried to request one that doesn't exist, which probably never happened prior to 4.17.

Thanks for the review.

Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.