Skip to contentSkip to conversation

enclosure: fix oops while iterating enclosure_status array

PATCH In review Jeff Mahoney 2 participants 4 messages Nov 20, 2009 – Nov 26, 2009

Subsystems (2)
SCSI Char and misc drivers
lore
Share & tools
lore

Jeff Mahoney

author
#0 lore raw

PATCH 1/1 enclosure: fix oops while iterating enclosure_status array

enclosure_status is expected to be a NULL terminated array of strings but isn't actually NULL terminated. When writing an invalid value to /sys/class/enclosure/.../.../status, it goes off the end of the array and Oopses.

This patch uses the array size instead.

Reported-by Artur Wojcik <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>
---
 drivers/misc/enclosure.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/misc/enclosure.c +3 -2
@@ -412,8 +412,9 @@ static ssize_t set_component_status(stru
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 	int i;
 
-	for (i = 0; enclosure_status[i]; i++) {
-		if (strncmp(buf, enclosure_status[i],
+	for (i = 0; i < ARRAY_SIZE(enclosure_status); i++) {
+		if (enclosure_status[i] &&
+		    strncmp(buf, enclosure_status[i],
 			    strlen(enclosure_status[i])) == 0 &&
 		    (buf[strlen(enclosure_status[i])] == '\n' ||
 		     buf[strlen(enclosure_status[i])] == '\0'))
Signature · 1 line
Jeff Mahoney
#1 lore raw

Adding linux-scsi to the cc list.

Patch looks fine to me, though, thanks.

James

Quoted 27 lines · Jeff Mahoney

enclosure_status is expected to be a NULL terminated array of strings
but isn't actually NULL terminated. When writing an invalid value to /sys/class/enclosure/.../.../status, it goes off the end of the array and Oopses.

This patch uses the array size instead.

Reported-by Artur Wojcik <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>
---
 drivers/misc/enclosure.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/misc/enclosure.c +3 -2
@@ -412,8 +412,9 @@ static ssize_t set_component_status(stru
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 	int i;
 
-	for (i = 0; enclosure_status[i]; i++) {
-		if (strncmp(buf, enclosure_status[i],
+	for (i = 0; i < ARRAY_SIZE(enclosure_status); i++) {
+		if (enclosure_status[i] &&
+		    strncmp(buf, enclosure_status[i],
 			    strlen(enclosure_status[i])) == 0 &&
 		    (buf[strlen(enclosure_status[i])] == '\n' ||
 		     buf[strlen(enclosure_status[i])] == '\0'))
#2 lore raw
In reply to Jeff Mahoney

*really* (promise) adding linux-scsi to the cc list this time.

Patch looks fine to me, though, thanks.

James

Quoted 27 lines · Jeff Mahoney

enclosure_status is expected to be a NULL terminated array of strings
but isn't actually NULL terminated. When writing an invalid value to /sys/class/enclosure/.../.../status, it goes off the end of the array and Oopses.

This patch uses the array size instead.

Reported-by Artur Wojcik <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>
---
 drivers/misc/enclosure.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/misc/enclosure.c +3 -2
@@ -412,8 +412,9 @@ static ssize_t set_component_status(stru
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 	int i;
 
-	for (i = 0; enclosure_status[i]; i++) {
-		if (strncmp(buf, enclosure_status[i],
+	for (i = 0; i < ARRAY_SIZE(enclosure_status); i++) {
+		if (enclosure_status[i] &&
+		    strncmp(buf, enclosure_status[i],
 			    strlen(enclosure_status[i])) == 0 &&
 		    (buf[strlen(enclosure_status[i])] == '\n' ||
 		     buf[strlen(enclosure_status[i])] == '\0'))
#3 lore raw
Quoted 26 lines · James Bottomley

*really* (promise) adding linux-scsi to the cc list this time.

Patch looks fine to me, though, thanks.

James

Quoted 19 lines · Jeff Mahoney

enclosure_status is expected to be a NULL terminated array of strings
but isn't actually NULL terminated. When writing an invalid value to /sys/class/enclosure/.../.../status, it goes off the end of the array and Oopses.

This patch uses the array size instead.

Reported-by Artur Wojcik <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>
---
 drivers/misc/enclosure.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/misc/enclosure.c +0 -0
@@ -412,8 +412,9 @@ static ssize_t set_component_status(stru
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 	int i;
Quoted 8 lines

drivers/misc/enclosure.c static ssize_t set_component_status(stru · line 420

Quoted 8 lines
drivers/misc/enclosure.c +3 -2
-	for (i = 0; enclosure_status[i]; i++) {
-		if (strncmp(buf, enclosure_status[i],
+	for (i = 0; i < ARRAY_SIZE(enclosure_status); i++) {
+		if (enclosure_status[i] &&
+		    strncmp(buf, enclosure_status[i],
 			    strlen(enclosure_status[i])) == 0 &&
 		    (buf[strlen(enclosure_status[i])] == '\n' ||
 		     buf[strlen(enclosure_status[i])] == '\0'))

Actually, it's not fine ... it's giving this error:

drivers/misc/enclosure.c: In function 'set_component_status':
drivers/misc/enclosure.c:453: warning: array subscript is above array bounds

It's another place where there's an assumption that the array is zero terminated.

I think the actual best fix is simply to make the assumption true, like the patch below.

James

---

drivers/misc/enclosure.c +1 -0
index e9eae4a..1eac626 100644
@@ -391,6 +391,7 @@ static const char *const enclosure_status [] = {
 	[ENCLOSURE_STATUS_NOT_INSTALLED] = "not installed",
 	[ENCLOSURE_STATUS_UNKNOWN] = "unknown",
 	[ENCLOSURE_STATUS_UNAVAILABLE] = "unavailable",
+	[ENCLOSURE_STATUS_MAX] = NULL,
 };
 
 static const char *const enclosure_type [] = {
include/linux/enclosure.h +2 -0
index 90d1c21..9a33c5f 100644
@@ -42,6 +42,8 @@ enum enclosure_status {
 	ENCLOSURE_STATUS_NOT_INSTALLED,
 	ENCLOSURE_STATUS_UNKNOWN,
 	ENCLOSURE_STATUS_UNAVAILABLE,
+	/* last element for counting purposes */
+	ENCLOSURE_STATUS_MAX
 };
 
 /* SFF-8485 activity light settings */