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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20201005070246.20927-1-warthog618@gmail.com>
Date:   Mon,  5 Oct 2020 15:02:46 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        bgolaszewski@...libre.com, linus.walleij@...aro.org
Cc:     Kent Gibson <warthog618@...il.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: [PATCH] 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 <andy.shevchenko@...il.com>
Signed-off-by: Kent Gibson <warthog618@...il.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