Skip to contentSkip to conversation

gpiolib: cdev: switch from kstrdup() to kstrndup()

PATCH In review Kent Gibson 2 participants 2 messages Oct 5, 2020 – Oct 7, 2020

Subsystems (2)
GPIO GPIO uapi
lore
Share & tools
lore

Kent Gibson

author reviewer
#0 lore raw

PATCH 1/1 gpiolib: cdev: switch from kstrdup() to kstrndup()

Use kstrndup() to copy line labels from the userspace provided char array, rather than ensuring the char array contains a null terminator and using kstrdup().

Note that the length provided to kstrndup() still assumes that the char array does contain a null terminator, so the maximum string length is one less than the array. This is consistent with the previous behaviour.

Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by Kent Gibson <[email protected]>

---

The change to kstrndup() was suggested by Andy as part of the review of the uAPI v2. This patch is my initial interpretion of that suggestion, applied to both the v2 case and the two corresponding v1 cases.

I have since realized that the consumer_label is copied back to userspace, so in the existing code the consumer_label, which may have been modified, is implicitly a return value. I doubt that is intentional or that it is used as such, but strictly speaking this change may break the v1 ABI?? If so, it will be necessary to restore the setting of the last array entry to '\0'.

Cheers,
Kent.

 drivers/gpio/gpiolib-cdev.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
drivers/gpio/gpiolib-cdev.c +13 -14
index 73386fcc252d..94733aab3224 100644
@@ -307,11 +307,11 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	lh->gdev = gdev;
 	get_device(&gdev->dev);
 
-	/* Make sure this is terminated */
-	handlereq.consumer_label[sizeof(handlereq.consumer_label)-1] = '\0';
-	if (strlen(handlereq.consumer_label)) {
-		lh->label = kstrdup(handlereq.consumer_label,
-				    GFP_KERNEL);
+	if (handlereq.consumer_label[0] != '\0') {
+		/* label is only initialized if consumer_label is set */
+		lh->label = kstrndup(handlereq.consumer_label,
+				     sizeof(handlereq.consumer_label) - 1,
+				     GFP_KERNEL);
 		if (!lh->label) {
 			ret = -ENOMEM;
 			goto out_free_lh;

@@ -1322,11 +1322,10 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 		INIT_DELAYED_WORK(&lr->lines[i].work, debounce_work_func);
 	}
 
-	/* Make sure this is terminated */
-	ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
-	if (strlen(ulr.consumer)) {
+	if (ulr.consumer[0] != '\0') {
 		/* label is only initialized if consumer is set */
-		lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
+		lr->label = kstrndup(ulr.consumer, sizeof(ulr.consumer) - 1,
+				     GFP_KERNEL);
 		if (!lr->label) {
 			ret = -ENOMEM;
 			goto out_free_linereq;

@@ -1711,11 +1710,11 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	le->gdev = gdev;
 	get_device(&gdev->dev);
 
-	/* Make sure this is terminated */
-	eventreq.consumer_label[sizeof(eventreq.consumer_label)-1] = '\0';
-	if (strlen(eventreq.consumer_label)) {
-		le->label = kstrdup(eventreq.consumer_label,
-				    GFP_KERNEL);
+	if (eventreq.consumer_label[0] != '\0') {
+		/* label is only initialized if consumer_label is set */
+		le->label = kstrndup(eventreq.consumer_label,
+				     sizeof(eventreq.consumer_label) - 1,
+				     GFP_KERNEL);
 		if (!le->label) {
 			ret = -ENOMEM;
 			goto out_free_le;

base-commit: 237d96164f2c2b33d0d5094192eb743e9e1b04ad

Signature · 1 line
2.28.0
#1 lore raw
Quoted 10 lines · Kent Gibson

Use kstrndup() to copy line labels from the userspace provided char array, rather than ensuring the char array contains a null terminator and using kstrdup().

Note that the length provided to kstrndup() still assumes that the char array does contain a null terminator, so the maximum string length is one less than the array. This is consistent with the previous behaviour.

Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by Kent Gibson <[email protected]>

General good practice! Patch applied.

Yours,
Linus Walleij