| Message ID | 20151019102005.GF26688@mwanda |
|---|---|
| State | Accepted |
| Commit | 2382960793c2480277ae98a891ea5aa566e06ff1 |
| Headers | show |
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
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
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
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.
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; }
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>