[patch] mtd/docg3: off by one in doc_register_sysfs()

[patch] mtd/docg3: off by one in doc_register_sysfs()

[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.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index f00d0da..c3a2695 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -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;
 }
 

Re: [patch] mtd/docg3: off by one in doc_register_sysfs()

[Robert Jarzmik]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> 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

Re: [patch] mtd/docg3: off by one in doc_register_sysfs()

[Dan Carpenter]

On Sat, Oct 24, 2015 at 11:49:27AM +0200, Robert Jarzmik wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > 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

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

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

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

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

regards,
dan carpenter

Re: [patch] mtd/docg3: off by one in doc_register_sysfs()

[Robert Jarzmik]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Sat, Oct 24, 2015 at 11:49:27AM +0200, Robert Jarzmik wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> 
>> > 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
>
> 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 <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [patch] mtd/docg3: off by one in doc_register_sysfs()

[Brian Norris]

On Sun, Oct 25, 2015 at 08:54:16AM +0100, Robert Jarzmik wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > On Sat, Oct 24, 2015 at 11:49:27AM +0200, Robert Jarzmik wrote:
> >> Dan Carpenter <dan.carpenter@oracle.com> writes:
> >> 
> >> > 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
> >
> > 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 <robert.jarzmik@free.fr>

Applied to l2-mtd.git. Thanks.