mtd/docg3: off by one in doc_register_sysfs()

PATCH Applied Dan Carpenter 3 participants 5 messages Oct 19, 2015 – Oct 26, 2015

Subsystems (3)

Kernel janitors Memory technology devices (MTD) Msystems diskonchip G3 MTD driver

lore

Share & tools

lore

Dan Carpenter

author

2015-10-19 10:20 UTC #0 lore raw

PATCH 1/1 mtd/docg3: off by one in doc_register_sysfs()

Smatch found a bug in the error handling:

	drivers/mtd/devices/docg3.c:1634 doc_register_sysfs()
	error: buffer overflow 'doc_sys_attrs' 4 <= 4

The problem is that if the very last device_create_file() fails, then we are beyond the end of the array. Actually, any time i = 3 then there is a problem. We can fix this an simplify the code at the same time by moving the !ret conditions out of the for loops and using a goto instead.

Signed-off-by Dan Carpenter <[email protected]>

drivers/mtd/devices/docg3.c +16 -6

index f00d0da..c3a2695 100644
@@ -1620,20 +1620,30 @@ static struct device_attribute doc_sys_attrs[DOC_MAX_NBFLOORS][4] = {
 static int doc_register_sysfs(struct platform_device *pdev,
 			      struct docg3_cascade *cascade)
 {
-	int ret = 0, floor, i = 0;
 	struct device *dev = &pdev->dev;
+	int floor;
+	int ret;
+	int i;
 
-	for (floor = 0; !ret && floor < DOC_MAX_NBFLOORS &&
-		     cascade->floors[floor]; floor++)
-		for (i = 0; !ret && i < 4; i++)
+	for (floor = 0;
+	     floor < DOC_MAX_NBFLOORS && cascade->floors[floor];
+	     floor++) {
+		for (i = 0; i < 4; i++) {
 			ret = device_create_file(dev, &doc_sys_attrs[floor][i]);
-	if (!ret)
-		return 0;
+			if (ret)
+				goto remove_files;
+		}
+	}
+
+	return 0;
+
+remove_files:
 	do {
 		while (--i >= 0)
 			device_remove_file(dev, &doc_sys_attrs[floor][i]);
 		i = 4;
 	} while (--floor >= 0);
+
 	return ret;
 }

Robert Jarzmik

maintainer

2015-10-24 09:49 UTC #1 lore raw

Quoted 10 lines · Dan Carpenter

Smatch found a bug in the error handling:
	drivers/mtd/devices/docg3.c:1634 doc_register_sysfs()
	error: buffer overflow 'doc_sys_attrs' 4 <= 4
The problem is that if the very last device_create_file() fails, then we are beyond the end of the array. Actually, any time i = 3 then there is a problem. We can fix this an simplify the code at the same time by moving the !ret conditions out of the for loops and using a goto instead.

Hi Dan,

I must admit I don't see the issue here :
- if the last device_create_file() fail, we have :
- i = 3, ret = -Exxx
- doc_sys_attrs[floor][0] is populated
- doc_sys_attrs[floor][1] is populated
- doc_sys_attrs[floor][2] is populated
- doc_sys_attrs[floor][3] is probably NULL
- next for loop exits

The while loop takes over :
- first iteration :
- --i => i = 2
device_remove_file(dev, &doc_sys_attrs[floor][2]);
- then the remaining attributes

I don't see the end of array issue. Could you tell me what I miss ?

Cheers.

--
Robert

Dan Carpenter

author

2015-10-24 17:49 UTC #2 lore raw

Quoted 15 lines · Robert Jarzmik

Quoted 10 lines · Dan Carpenter
Smatch found a bug in the error handling:
	drivers/mtd/devices/docg3.c:1634 doc_register_sysfs()
	error: buffer overflow 'doc_sys_attrs' 4 <= 4
The problem is that if the very last device_create_file() fails, then we are beyond the end of the array. Actually, any time i = 3 then there is a problem. We can fix this an simplify the code at the same time by moving the !ret conditions out of the for loops and using a goto instead.
Hi Dan,

Quoted 7 lines

I must admit I don't see the issue here :
- if the last device_create_file() fail, we have :
- i = 3, ret = -Exxx
- doc_sys_attrs[floor][0] is populated
- doc_sys_attrs[floor][1] is populated
- doc_sys_attrs[floor][2] is populated
- doc_sys_attrs[floor][3] is probably NULL

We increment "i" to 4.
We increment "floor" here before the next loop exits.

Quoted 5 lines

- next for loop exits
The while loop takes over :
- first iteration :
- --i => i = 2

Actually --i is 3 and "floor" is out of bounds.

Quoted 3 lines

	device_remove_file(dev, &doc_sys_attrs[floor][2]);
- then the remaining attributes

regards,
dan carpenter

Robert Jarzmik

maintainer Acked-by

2015-10-25 07:54 UTC #3 lore raw

Quoted 24 lines · Dan Carpenter

Quoted 15 lines · Robert Jarzmik
Quoted 10 lines · Dan Carpenter
Smatch found a bug in the error handling:
	drivers/mtd/devices/docg3.c:1634 doc_register_sysfs()
	error: buffer overflow 'doc_sys_attrs' 4 <= 4
The problem is that if the very last device_create_file() fails, then we are beyond the end of the array. Actually, any time i = 3 then there is a problem. We can fix this an simplify the code at the same time by moving the !ret conditions out of the for loops and using a goto instead.
Hi Dan,
Quoted 7 lines
I must admit I don't see the issue here :
- if the last device_create_file() fail, we have :
- i = 3, ret = -Exxx
- doc_sys_attrs[floor][0] is populated
- doc_sys_attrs[floor][1] is populated
- doc_sys_attrs[floor][2] is populated
- doc_sys_attrs[floor][3] is probably NULL

Quoted 1 line

We increment "i" to 4.

Ah yes, I see it now, thanks. Somehow in my brain the !ret condition in the for loop was preventing the increment ... silly.

So:

Acked-by Robert Jarzmik <[email protected]>

Cheers.

--
Robert

Brian Norris

Applied

2015-10-26 18:45 UTC #4 lore raw

Quoted 26 lines · Robert Jarzmik

Quoted 24 lines · Dan Carpenter
Quoted 22 lines · Robert Jarzmik
Quoted 10 lines · Dan Carpenter
Smatch found a bug in the error handling:
	drivers/mtd/devices/docg3.c:1634 doc_register_sysfs()
	error: buffer overflow 'doc_sys_attrs' 4 <= 4
The problem is that if the very last device_create_file() fails, then we are beyond the end of the array. Actually, any time i = 3 then there is a problem. We can fix this an simplify the code at the same time by moving the !ret conditions out of the for loops and using a goto instead.
Hi Dan,
I must admit I don't see the issue here :
- if the last device_create_file() fail, we have :
- i = 3, ret = -Exxx
- doc_sys_attrs[floor][0] is populated
- doc_sys_attrs[floor][1] is populated
- doc_sys_attrs[floor][2] is populated
- doc_sys_attrs[floor][3] is probably NULL

Quoted 6 lines

Quoted 1 line
We increment "i" to 4.
Ah yes, I see it now, thanks. Somehow in my brain the !ret condition in the for loop was preventing the increment ... silly.
So:
Acked-by Robert Jarzmik <[email protected]>

Applied to l2-mtd.git. Thanks.