potential overflow in de4x5.c
Subsystems (2)
Hi I found this using smatch (http://repo.or.cz/w/smatch.git).
drivers/net/tulip/de4x5.c
4772 lp->active = *p++; 4773 if (MOTO_SROM_BUG) lp->active = 0; 4774 lp->phy[lp->active].gep = (*p ? p : NULL); p += (2 * (*p) + 1);
lp->phy is an array of size 8.
MOTO_SROM_BUG is defined like this.
#define MOTO_SROM_BUG (lp->active == 8 && (get_unaligned_le32(dev->dev_addr) & 0x00ffffff) == 0x3e0008)
If lp->active == 8 then we have a buffer overflow.
regards,
dan carpenter
Quoted 7 lines · Dan Carpenter
Hi I found this using smatch (http://repo.or.cz/w/smatch.git).
drivers/net/tulip/de4x5.c4772 lp->active = *p++; 4773 if (MOTO_SROM_BUG) lp->active = 0; 4774 lp->phy[lp->active].gep = (*p ? p : NULL); p += (2 * (*p) + 1);
Quoted 7 lines
lp->phyis an array of size 8.
MOTO_SROM_BUGis defined like this.#define
MOTO_SROM_BUG(lp->active== 8 && (get_unaligned_le32(dev->dev_addr)&0x00ffffff) ==0x3e0008)If
lp->active== 8 then we have a buffer overflow.
Dan,
When does the overflow actually occur?
That code is reseting the value to work around a specific SROM bug:
http://lists.ozlabs.org/pipermail/linuxppc-dev/1999-March/001421.html
If you want to make the "input validation" more robust, that would be fine with me. But smatch hasn't convinced me there is a bug here.
thanks,
grant
Quoted 3 lines
regards,
dan carpenter
Quoted 16 lines · Grant Grundler
Quoted 7 lines · Dan Carpenter
Hi I found this using smatch (http://repo.or.cz/w/smatch.git).
drivers/net/tulip/de4x5.c4772 lp->active = *p++; 4773 if (MOTO_SROM_BUG) lp->active = 0; 4774 lp->phy[lp->active].gep = (*p ? p : NULL); p += (2 * (*p) + 1);Quoted 7 lines
lp->phyis an array of size 8.
MOTO_SROM_BUGis defined like this.#define
MOTO_SROM_BUG(lp->active== 8 && (get_unaligned_le32(dev->dev_addr)&0x00ffffff) ==0x3e0008)If
lp->active== 8 then we have a buffer overflow.
Quoted 8 lines
Dan,
When does the overflow actually occur?That code is reseting the value to work around a specific SROM bug:
http://lists.ozlabs.org/pipermail/linuxppc-dev/1999-March/001421.htmlIf you want to make the "input validation" more robust, that would be fine with me. But smatch hasn't convinced me there is a bug here.
BTW, someone suggested to fix up this same bit of code before:
http://www.mail-archive.com/[email protected]/msg09838.html
And I'm not sure why that patch wasn't accepted then either. Patch looks fine to me.
thanks,
grant
Quoted 13 lines · Grant Grundler
Quoted 12 lines · Grant Grundler
Quoted 11 lines · Dan Carpenter
Hi I found this using smatch (http://repo.or.cz/w/smatch.git).
drivers/net/tulip/de4x5.c4772 lp->active = *p++; 4773 if (MOTO_SROM_BUG) lp->active = 0; 4774 lp->phy[lp->active].gep = (*p ? p : NULL); p += (2 * (*p) + 1);
lp->phyis an array of size 8.
MOTO_SROM_BUGis defined like this.
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
#define
MOTO_SROM_BUG(lp->active== 8 && (get_unaligned_le32(dev->dev_addr)&0x00ffffff) ==0x3e0008)If
lp->active== 8 then we have a buffer overflow.Dan,
When does the overflow actually occur?
You may be right that it can't happen in real life but from reading the code without access to the hardware, it looks like it would happen on line 4774 quoted above.
Quoted 6 lines
Quoted 5 lines
That code is reseting the value to work around a specific SROM bug:
http://lists.ozlabs.org/pipermail/linuxppc-dev/1999-March/001421.htmlIf you want to make the "input validation" more robust, that would be fine with me. But smatch hasn't convinced me there is a bug here.
Basically the MOTO_SROM_BUG macro is asking: Do we have an array overflow and a hardware bug? If so we had better do something about the hardware bug. It sounds silly to me.
Quoted 5 lines
BTW, someone suggested to fix up this same bit of code before:
http://www.mail-archive.com/[email protected]/msg09838.htmlAnd I'm not sure why that patch wasn't accepted then either. Patch looks fine to me.
Someone has updated the code since he posted the patch, presumably to fix the second overflow he mentioned.
There is still another one left unfixed though which smatch misses.
5073 if ((j == limit) && (i < DE4X5_MAX_MII)) {
5074 for (k=0; k < DE4X5_MAX_PHY && lp->phy[k].id; k++);
5075 lp->phy[k].addr = i;k could be == DE4X5_MAX_PHY on line 5075.
regards,
dan carpenter
Quoted 2 lines
thanks,
grant
Quoted 1 line · Dan Carpenter
Quoted 1 line
Quoted 1 line
Quoted 1 line
#define
MOTO_SROM_BUG(lp->active== 8 && (get_unaligned_le32(dev->dev_addr)&0x00ffffff) ==0x3e0008)
...
Quoted 3 lines
Basically the
MOTO_SROM_BUGmacro is asking: Do we have an array overflow and a hardware bug? If so we had better do something about the hardware bug. It sounds silly to me.
Hardware bug? A firmware bug I think.
I read the MOTO_SROM_BUG to be using both "active" and "dev_addr" to be certain it was dealing with a broken SROM. And then fixing up the "bork3d" values reported by the SROM (setting active to 0).
This still leaves open the question about when lp->active could be >= DE4X5_MAX_MII.
Quoted 8 lines
Quoted 5 lines
BTW, someone suggested to fix up this same bit of code before:
http://www.mail-archive.com/[email protected]/msg09838.htmlAnd I'm not sure why that patch wasn't accepted then either. Patch looks fine to me.
Someone has updated the code since he posted the patch, presumably to fix the second overflow he mentioned.
*nod*
Quoted 8 lines
There is still another one left unfixed though which smatch misses.
5073 if ((j == limit) && (i < DE4X5_MAX_MII)) { 5074 for (k=0; k < DE4X5_MAX_PHY && lp->phy[k].id; k++); 5075 lp->phy[k].addr = i;k could be ==
DE4X5_MAX_PHYon line 5075.
Yup. In theory at least. But can anyone point me at a DE4X5 device that could have 7 or more phys attached to it?
I expect no more than three cases (thin_lan Coax, RJ45, MAU) but am probably missing a few others - unlikely more than one or two more.
One unlikely but possible case: broken HW which reads ~0U (PCI Master Abort) for phy[] values.
cheers,
grant