Hi:
I noticed a bug in the realloc error checking code in the QEMU backend
for RealTek8139. However, what''s worse is that there is no cap on the
total size of the transmit buffer at all. So a guest can keep extending
it until memory runs out.
CP_TX_BUFFER_SIZE is already 64K. So it seems to me that we don''t need
the while loop to extend the buffer at all since no transmitted packet
should be anywhere near this size.
Are there any objections to getting rid of the following while loop
altogether and replacing it with a straight failure?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff -r f026d4091322 tools/ioemu/hw/rtl8139.c
--- a/tools/ioemu/hw/rtl8139.c Tue Nov 14 18:52:58 2006 +0000
+++ b/tools/ioemu/hw/rtl8139.c Wed Nov 15 15:35:24 2006 +1100
@@ -2001,8 +2001,13 @@ static int rtl8139_cplus_transmit_one(RT
while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize
>= s->cplus_txbuffer_len)
{
+ void *txbuffer;
+
s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
- s->cplus_txbuffer = realloc(s->cplus_txbuffer,
s->cplus_txbuffer_len);
+ txbuffer = realloc(s->cplus_txbuffer, s->cplus_txbuffer_len);
+ if (!txbuffer)
+ free(s->cplus_txbuffer);
+ s->cplus_txbuffer = txbuffer;
DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space
changed to %d\n", s->cplus_txbuffer_len));
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On Wed, Nov 15, 2006 at 03:38:27PM +1100, herbert wrote:> > CP_TX_BUFFER_SIZE is already 64K. So it seems to me that we don''t need > the while loop to extend the buffer at all since no transmitted packet > should be anywhere near this size. > > Are there any objections to getting rid of the following while loop > altogether and replacing it with a straight failure?Since I haven''t heard any objections, here is a patch to do just that. [QEMU] rtl8139: Disallow chaining above 64K As it stands the 8139C+ TX chaining is only bounded by realloc failure. This is contrary to how the real hardware operates. It also has DoS potential when ioemu runs in dom0. This patch makes any attempt to chain a frame beyond 64K fail immediately. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- diff -r 5f7b5e5ca14b tools/ioemu/hw/rtl8139.c --- a/tools/ioemu/hw/rtl8139.c Thu Nov 16 17:07:23 2006 +0000 +++ b/tools/ioemu/hw/rtl8139.c Fri Nov 17 11:24:34 2006 +1100 @@ -1999,12 +1999,12 @@ static int rtl8139_cplus_transmit_one(RT DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer allocated space %d\n", s->cplus_txbuffer_len)); } - while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len) - { - s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE; - s->cplus_txbuffer = realloc(s->cplus_txbuffer, s->cplus_txbuffer_len); - - DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space changed to %d\n", s->cplus_txbuffer_len)); + if (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len) + { + free(s->cplus_txbuffer); + s->cplus_txbuffer = NULL; + + DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space exceeded: %d\n", s->cplus_txbuffer_offset + txsize)); } if (!s->cplus_txbuffer) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, Nov 17, 2006 at 11:29:45AM +1100, herbert wrote:> > Since I haven''t heard any objections, here is a patch to do just that.In the interest of diffing things twice, here is a more complete patch. [QEMU] rtl8139: Disallow chaining above 64K As it stands the 8139C+ TX chaining is only bounded by realloc failure. This is contrary to how the real hardware operates. It also has DoS potential when ioemu runs in dom0. This patch makes any attempt to chain a frame beyond 64K fail immediately. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- diff -r 5f7b5e5ca14b tools/ioemu/hw/rtl8139.c --- a/tools/ioemu/hw/rtl8139.c Thu Nov 16 17:07:23 2006 +0000 +++ b/tools/ioemu/hw/rtl8139.c Fri Nov 17 11:24:34 2006 +1100 @@ -1999,12 +1999,12 @@ static int rtl8139_cplus_transmit_one(RT DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer allocated space %d\n", s->cplus_txbuffer_len)); } - while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len) - { - s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE; - s->cplus_txbuffer = realloc(s->cplus_txbuffer, s->cplus_txbuffer_len); - - DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space changed to %d\n", s->cplus_txbuffer_len)); + if (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len) + { + free(s->cplus_txbuffer); + s->cplus_txbuffer = NULL; + + DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space exceeded: %d\n", s->cplus_txbuffer_offset + txsize)); } if (!s->cplus_txbuffer) diff -r 5f7b5e5ca14b tools/ioemu/patches/qemu-rtl8139-max-frame-size --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tools/ioemu/patches/qemu-rtl8139-max-frame-size Fri Nov 17 11:43:14 2006 +1100 @@ -0,0 +1,22 @@ +diff -r 5f7b5e5ca14b tools/ioemu/hw/rtl8139.c +--- ioemu/hw/rtl8139.c Thu Nov 16 17:07:23 2006 +0000 ++++ ioemu/hw/rtl8139.c Fri Nov 17 11:24:34 2006 +1100 +@@ -1999,12 +1999,12 @@ static int rtl8139_cplus_transmit_one(RT + DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer allocated space %d\n", s->cplus_txbuffer_len)); + } + +- while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len) +- { +- s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE; +- s->cplus_txbuffer = realloc(s->cplus_txbuffer, s->cplus_txbuffer_len); +- +- DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space changed to %d\n", s->cplus_txbuffer_len)); ++ if (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len) ++ { ++ free(s->cplus_txbuffer); ++ s->cplus_txbuffer = NULL; ++ ++ DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer space exceeded: %d\n", s->cplus_txbuffer_offset + txsize)); + } + + if (!s->cplus_txbuffer) diff -r 5f7b5e5ca14b tools/ioemu/patches/series --- a/tools/ioemu/patches/series Thu Nov 16 17:07:23 2006 +0000 +++ b/tools/ioemu/patches/series Fri Nov 17 11:45:11 2006 +1100 @@ -53,3 +53,4 @@ hypervisor-rtc hypervisor-rtc ide-cd-dma vnc-password +qemu-rtl8139-max-frame-size _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Igor Kovalenko
2006-Dec-21 23:11 UTC
[Xen-devel] Re: [Qemu-devel] Re: qemu/rtl8139: Max transmit frame size
On 11/17/06, Herbert Xu <herbert@gondor.apana.org.au> wrote:> > On Wed, Nov 15, 2006 at 03:38:27PM +1100, herbert wrote: > > > > CP_TX_BUFFER_SIZE is already 64K. So it seems to me that we don''t need > > the while loop to extend the buffer at all since no transmitted packet > > should be anywhere near this size. > > > > Are there any objections to getting rid of the following while loop > > altogether and replacing it with a straight failure? > > Since I haven''t heard any objections, here is a patch to do just that. > > [QEMU] rtl8139: Disallow chaining above 64K > > As it stands the 8139C+ TX chaining is only bounded by realloc failure. > This is contrary to how the real hardware operates. It also has DoS > potential when ioemu runs in dom0. > > This patch makes any attempt to chain a frame beyond 64K fail immediately. >True, a limit would be useful. It may be possible to start a chain at first TX descriptor and keep feeding the card with new buffers while hardware is sending older ones, without underrun, if checksum offloading is disabled. Emulation is assembling a complete packet to feed slirp routine - so there is another limit on packet size, the one slirp is able to handle. It would be nice to know what is the actual hardware limit for chaining. Only reference to 64K I found in docs is referring to receive ring buffer size in "C" mode. -- Kind Regards, Igor V. Kovalenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel