This is the mail archive of the ecos-discuss@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: bug in eth_drv_send (memory overwrite)


Please keep your replies on the mailing list so that all may benefit
- private email is available only with a support contract.

On Wed, 2006-01-18 at 15:16 +0100, Deak, Ferenc wrote:
> > On Wed, 2006-01-18 at 13:18 +0100, Deak, Ferenc wrote:
> > > Hi!
> > > 
> > > I think I found a problem in io/eth/current/src/net/eth_drv.c in eth_drv_send
> > > I use the current CVS.
> > > 
> > > In eth_drv.c at line 706 there is a check 
> > > "if ( MAX_ETH_DRV_SG < sg_len )"
> > > I think it have to be
> > > "if ( MAX_ETH_DRV_SG <= sg_len )"
> > > 
> > > for explanation see the snippet at the end of teh file from eth_drv.c, look for lines containing !!!!!
> > > 
> > > By the way is there any better solution for the problem (MAX_ETH_DRV_SG <= sg_len) 
> > > than simply "drop it on the floor" (comment from the file). It's not too nice!
> > > 
> > 
> > No, if you change the '<' to '<=', you'd stop one short of using
> > all of the list.
> 
> I have looked it over again, but in my opinion sg_list[MAX_ETH_DRV_SG] *IS WRITTEN*, and 
> it is declared as "struct eth_drv_sg sg_list[MAX_ETH_DRV_SG];"
> You can't write "array[N]", if it is declared as "sometype array[N]".

But the check is made *after* the list length has been incremented.  It
is valid for sg_len to be equal to MAX_ETH_DRV_SG at this point.

> 
> > As for what to do, if I had good ideas, they'd already be in there.
> > The chances of this are extremely remote (never been seen in the wild!)
> > and to repair all one would need to do is increase the size of the list.
> > It's kept small just to save space, but in reality it only takes up
> > 8 bytes per slot, so an increase would be minimal.  Of course, the
> > default size is tunable using CDL.
> 
> Yes I know, and we have tuned it already.
> We have reached the limit of this list (MAX_ETH_DRV_SG). It is possibly a desing error, 
                                                                            ??????

> but this is a valid code, calling a lot of write on a socket with len<50. We will redesign 
> it, but now increasing MAX_ETH_DRV_SG is the quick solution. Perhaps the diag_printf
> or a comment here could be more explanatory?
> 

How much more explanation do you need?  It tells you that the list was
too small and to satisfy the requirements, you'd need "needed" items
in the list.

> > 
> > > 
> > > 
> > >         sg_len = 0;  total_len = 0;
> > >         for (m = m0; m ; m = m->m_next) {
> > >             data = mtod(m, u_char *);
> > >             len = m->m_len;
> > >             total_len += len;
> > >             sg_list[sg_len].buf = (CYG_ADDRESS)data;
> > >             sg_list[sg_len].len = len;	// !!!!!! largest sg_len here is MAX_ETH_DRV_SG
> > >             if ( len )
> > >                 sg_len++;			// !!!!!! largest sg_len here is MAX_ETH_DRV_SG+1 (after ++)
> > > #ifdef CYGDBG_IO_ETH_DRIVERS_DEBUG
> > >             if (cyg_io_eth_net_debug) {
> > >                 START_CONSOLE();
> > >                 diag_printf("xmit %d bytes at %p sg[%d]\n", len, data, sg_len);
> > >                 if ( cyg_io_eth_net_debug > 1)
> > >                     diag_dump_buf(data, len);
> > >                 END_CONSOLE();
> > >             }
> > > #endif
> > >             if ( MAX_ETH_DRV_SG < sg_len ) { // !!!!!! true at sg_len = MAX_ETH_DRV_SG+1
> > > #ifdef CYGPKG_IO_ETH_DRIVERS_WARN_NO_MBUFS
> > >                 int needed = 0;
> > >                 struct mbuf *m1;
> > >                 for (m1 = m0; m1 ; m1 = m1->m_next) needed++;
> > >                 START_CONSOLE();
> > >                 diag_printf("too many mbufs to tx, %d > %d, need %d\n", 
> > >                             sg_len, MAX_ETH_DRV_SG, needed );
> > >                 END_CONSOLE();
> > > #endif
> > >                 sg_len = 0;
> > >                 break; // drop it on the floor
> > >             }
> > >         }
> > > 
> > > Best regards,
> > > Ferenc Deak
> > > 
> > 
> > -- 
> > ------------------------------------------------------------
> > Gary Thomas                 |  Consulting for the
> > MLB Associates              |    Embedded world
> > ------------------------------------------------------------
> > 
-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]