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)


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

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

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


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