potential overflow in de4x5.c

potential overflow in de4x5.c

[Dan Carpenter]

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

Re: potential overflow in de4x5.c

[Grant Grundler]

On Sun, Jan 03, 2010 at 12:13:56PM +0200, Dan Carpenter wrote:
> 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.

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

> 
> regards,
> dan carpenter

Re: potential overflow in de4x5.c

[Grant Grundler]

On Mon, Jan 04, 2010 at 12:28:44AM -0700, Grant Grundler wrote:
> On Sun, Jan 03, 2010 at 12:13:56PM +0200, Dan Carpenter wrote:
> > 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.
> 
> 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.

BTW, someone suggested to fix up this same bit of code before:
   http://www.mail-archive.com/netdev@vger.kernel.org/msg09838.html

And I'm not sure why that patch wasn't accepted then either. Patch looks fine to me.

thanks,
grant

Re: potential overflow in de4x5.c

[Dan Carpenter]

On Mon, Jan 04, 2010 at 12:35:14AM -0700, Grant Grundler wrote:
> On Mon, Jan 04, 2010 at 12:28:44AM -0700, Grant Grundler wrote:
> > On Sun, Jan 03, 2010 at 12:13:56PM +0200, Dan Carpenter wrote:
> > > 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.
> > 
> > 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.

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

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.

> BTW, someone suggested to fix up this same bit of code before:
>    http://www.mail-archive.com/netdev@vger.kernel.org/msg09838.html
> 
> And 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

> thanks,
> grant

Re: potential overflow in de4x5.c

[Grant Grundler]

On Thu, Jan 07, 2010 at 06:40:07PM +0300, Dan Carpenter wrote:
> > > > #define MOTO_SROM_BUG    (lp->active == 8 && (get_unaligned_le32(dev->dev_addr) & 0x00ffffff) == 0x3e0008)
...
> 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.

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. 


> > BTW, someone suggested to fix up this same bit of code before:
> >    http://www.mail-archive.com/netdev@vger.kernel.org/msg09838.html
> > 
> > And 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*

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

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