[PATCH] gpiolib: cdev: switch from kstrdup() to kstrndup()

[PATCH] gpiolib: cdev: switch from kstrdup() to kstrndup()

[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 <andy.shevchenko@gmail.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

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(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 73386fcc252d..94733aab3224 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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
-- 
2.28.0

Re: [PATCH] gpiolib: cdev: switch from kstrdup() to kstrndup()

[Linus Walleij]

On Mon, Oct 5, 2020 at 9:03 AM Kent Gibson <warthog618@gmail.com> wrote:

> 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 <andy.shevchenko@gmail.com>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

General good practice! Patch applied.

Yours,
Linus Walleij