This is the mail archive of the
ecos-discuss@sourceware.org
mailing list for the eCos project.
Re: bug in eth_drv_send (memory overwrite)
- From: Gary Thomas <gary at mlbassoc dot com>
- To: "Deak, Ferenc" <ferenc dot deak at stp dot hu>
- Cc: eCos Discussion <ecos-discuss at ecos dot sourceware dot org>
- Date: Wed, 18 Jan 2006 07:23:26 -0700
- Subject: Re: [ECOS] bug in eth_drv_send (memory overwrite)
- References: <20060118131858.bde93ec6.ferenc.deak@stp.hu> <1137589588.28008.83.camel@hermes> <20060118151607.2701ea1f.ferenc.deak@stp.hu>
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