Skip to contentSkip to conversation

gpio-hammer: Avoid potential overflow in main

PATCH In review Gabriel Ravier 2 participants 8 messages Mar 12, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 gpio-hammer: Avoid potential overflow in main

If '-o' was used more than 64 times in a single invocation of gpio-hammer, this could lead to an overflow of the 'lines' array. This commit fixes this by avoiding the overflow and giving a proper diagnostic back to the user

---
 tools/gpio/gpio-hammer.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)
tools/gpio/gpio-hammer.c +17 -2
index 0e0060a6e..273d33847 100644
@@ -77,7 +77,7 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 
 		fprintf(stdout, "[%c] ", swirr[j]);
 		j++;
-		if (j == sizeof(swirr)-1)
+		if (j == sizeof(swirr) - 1)
 			j = 0;
 
 		fprintf(stdout, "[");
@@ -135,7 +135,14 @@ int main(int argc, char **argv)
 			device_name = optarg;
 			break;
 		case 'o':
-			lines[i] = strtoul(optarg, NULL, 10);
+			/*
+			 * Avoid overflow. Do not immediately error, we want to
+			 * be able to accurately report on the amount of times
+			 *'-o' was given to give an accurate error message
+			 */
+			if (i < GPIOHANDLES_MAX)
+				lines[i] = strtoul(optarg, NULL, 10);
+
 			i++;
 			break;
 		case '?':
@@ -143,6 +150,14 @@ int main(int argc, char **argv)
 			return -1;
 		}
 	}
+
+	if (i >= GPIOHANDLES_MAX) {
+		fprintf(stderr,
+			"Only %d occurences of '-o' are allowed, %d were found\n",
+			GPIOHANDLES_MAX, i + 1);
+		return -1;
+	}
+
 	nlines = i;
 
 	if (!device_name || !nlines) {
Signature · 1 line
2.24.1
#1 lore raw

czw., 12 mar 2020 o 10:40 Gabriel Ravier <[email protected]> napisał(a):

Quoted 16 lines

If '-o' was used more than 64 times in a single invocation of gpio-hammer, this could lead to an overflow of the 'lines' array. This commit fixes this by avoiding the overflow and giving a proper diagnostic back to the user

---
 tools/gpio/gpio-hammer.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)
tools/gpio/gpio-hammer.c +0 -0
index 0e0060a6e..273d33847 100644

tools/gpio/gpio-hammer.c int hammer_device(const char *device_name, unsigned int *lines, int nlines, · line 80

Quoted 5 lines
tools/gpio/gpio-hammer.c +1 -1
@@ -77,7 +77,7 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 
                fprintf(stdout, "[%c] ", swirr[j]);
                j++;
-               if (j == sizeof(swirr)-1)
+               if (j == sizeof(swirr) - 1)

Please don't try to sneak in unrelated changes into commits. This is of course correct coding-style-wise but send it in a separate patch.

Quoted 35 lines
tools/gpio/gpio-hammer.c +16 -2
                        j = 0;
 
                fprintf(stdout, "[");
@@ -135,7 +135,14 @@ int main(int argc, char **argv)
                        device_name = optarg;
                        break;
                case 'o':
-                       lines[i] = strtoul(optarg, NULL, 10);
+                       /*
+                        * Avoid overflow. Do not immediately error, we want to
+                        * be able to accurately report on the amount of times
+                        *'-o' was given to give an accurate error message
+                        */
+                       if (i < GPIOHANDLES_MAX)
+                               lines[i] = strtoul(optarg, NULL, 10);
+
                        i++;
                        break;
                case '?':
@@ -143,6 +150,14 @@ int main(int argc, char **argv)
                        return -1;
                }
        }
+
+       if (i >= GPIOHANDLES_MAX) {
+               fprintf(stderr,
+                       "Only %d occurences of '-o' are allowed, %d were found\n",
+                       GPIOHANDLES_MAX, i + 1);
+               return -1;
+       }
+
        nlines = i;
 
        if (!device_name || !nlines) {
--
Quoted 2 lines

2.24.1

Other than that, looks good.

Bartosz

#2 lore raw

Ah, that was accidental. I was applying scripts/Lindent to my code and ended up also having it applied to part of the old code. Didn't think it would hurt, but I guess it makes sense to be this stringent on separating logical changes. Will send a (complete) corrected patch in-reply-to this message.

#3 lore raw

PATCH 1/1 gpio-hammer: Avoid potential overflow in main

If '-o' was used more than 64 times in a single invocation of gpio-hammer, this could lead to an overflow of the 'lines' array. This commit fixes this by avoiding the overflow and giving a proper diagnostic back to the user

---
 tools/gpio/gpio-hammer.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
tools/gpio/gpio-hammer.c +16 -1
index 0e0060a6e..d0be21af1 100644
@@ -135,7 +135,14 @@ int main(int argc, char **argv)
 			device_name = optarg;
 			break;
 		case 'o':
-			lines[i] = strtoul(optarg, NULL, 10);
+			/*
+			 * Avoid overflow. Do not immediately error, we want to
+			 * be able to accurately report on the amount of times
+			 *'-o' was given to give an accurate error message
+			 */
+			if (i < GPIOHANDLES_MAX)
+				lines[i] = strtoul(optarg, NULL, 10);
+
 			i++;
 			break;
 		case '?':
@@ -143,6 +150,14 @@ int main(int argc, char **argv)
 			return -1;
 		}
 	}
+
+	if (i >= GPIOHANDLES_MAX) {
+		fprintf(stderr,
+			"Only %d occurences of '-o' are allowed, %d were found\n",
+			GPIOHANDLES_MAX, i + 1);
+		return -1;
+	}
+
 	nlines = i;
 
 	if (!device_name || !nlines) {
Signature · 1 line
2.24.1
#4 lore raw
In reply to Gabriel Ravier

czw., 12 mar 2020 o 15:21 Gabriel Ravier <[email protected]> napisał(a):

Quoted 7 lines

Ah, that was accidental. I was applying scripts/Lindent to my code and ended up also having it applied to part of the old code. Didn't think it would hurt, but I guess it makes sense to be this stringent on separating logical changes. Will send a (complete) corrected patch in-reply-to this message.

Please don't send patches in response to other threads. Always start a new thread and increment the version in [PATCH vX] tag.

Bart

#5 lore raw

Ah, seems like I didn't read the guide to getting code into the kernel thoroughly enough. Should I send the patch yet again just with a v2 in the subject header or is there no need to bother with that ?

Quoted 11 lines · Bartosz Golaszewski

czw., 12 mar 2020 o 15:21 Gabriel Ravier <[email protected]> napisał(a):

Quoted 6 lines

Ah, that was accidental. I was applying scripts/Lindent to my code and ended up also having it applied to part of the old code. Didn't think it would hurt, but I guess it makes sense to be this stringent on separating logical changes. Will send a (complete) corrected patch in-reply-to this message.

Please don't send patches in response to other threads. Always start a new thread and increment the version in [PATCH vX] tag.

Bart

#6 lore raw

czw., 12 mar 2020 o 15:33 Gabriel Ravier <[email protected]> napisał(a):

Quoted 5 lines

Ah, seems like I didn't read the guide to getting code into the kernel thoroughly enough. Should I send the patch yet again just with a v2 in the subject header or is there no need to bother with that ?

Please do so that patchwork picks it up. Also: please don't top-post on LKML ie. respond below others' text.

Bart

#7 lore raw
Quoted 9 lines · Bartosz Golaszewski

czw., 12 mar 2020 o 15:33 Gabriel Ravier <[email protected]> napisał(a):

Quoted 4 lines

Ah, seems like I didn't read the guide to getting code into the kernel thoroughly enough. Should I send the patch yet again just with a v2 in the subject header or is there no need to bother with that ?

Please do so that patchwork picks it up. Also: please don't top-post on LKML ie. respond below others' text.

Bart

Ok, will send it with s/PATCH/PATCH v2 ASAP.

Btw, is the "top-posting" the reason why lkml.org doesn't seem to be able to find my replies to your posts ?