[Qemu-devel] [PATCH] Fix for a malloc heap corruption problem in the slirp network code

[Qemu-devel] [PATCH] Fix for a malloc heap corruption problem in the slirp network code

[Juergen Keil]

Compiling inside a NetBSD 1.5 qemu guest OS (source files are located
on an NFS filesystem mounted from the Solaris host OS) crashes qemu
with a malloc heap corruption error, when the slirp user mode
networking code is in use.


Solaris' umem malloc library reports that a block freed by a call to
realloc (from function m_inc()) is still being used, when it
shouldn't:

% mdb i386-softmmu/qemu core
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::umem_status
Status:         ready and active
Concurrency:    2
Logs:           transaction=64k (inactive)
Message buffer:
umem allocator: boundary tag corrupted
bcp ^ bxstat = 2c694858, should be f4eef4ee
buffer=1977f000  bufctl=19713340  cache: umem_alloc_8192
previous transaction on buffer 1977f000:
thread=1  time=T-0.009171995  slab=8f9b2c0  cache: umem_alloc_8192
libumem.so.1'?? (0xd1f99c49)
libumem.so.1'umem_cache_free+0x3f
libumem.so.1'umem_free+0xd5
libumem.so.1'?? (0xd1f97c05)
libumem.so.1'free+0x14
libumem.so.1'realloc+0x66
qemu'm_inc+0x26
qemu'm_cat+0x4b
qemu'ip_reass+0x321
qemu'ip_input+0x261
qemu'slirp_input+0x6f
qemu'?? (0x8065055)
qemu'qemu_send_packet+0x13
qemu'?? (0x807d116)
qemu'cpu_outb+0x1b
umem: heap corruption detected
stack trace:
libumem.so.1'?? (0xd1f96099)
libumem.so.1'?? (0xd1f98b1c)
libumem.so.1'?? (0xd1f99ad2)
libumem.so.1'umem_cache_alloc+0xe1
libumem.so.1'umem_alloc+0x3f
libumem.so.1'malloc+0x23
qemu'm_inc+0x41
qemu'm_cat+0x4b
qemu'ip_reass+0x321
qemu'ip_input+0x261
qemu'slirp_input+0x6f
qemu'?? (0x8065055)
qemu'qemu_send_packet+0x13
qemu'?? (0x807d116)
qemu'cpu_outb+0x1b
qemu'code_gen_buffer+0x1103bd
qemu'main_loop+0x22
qemu'main+0x10ce
qemu'_start+0x5d

>


Using the "electric fence" memory allocator, the location of the data
corruption can be narrowed down to the destination address in the memcpy
call in slirp/mbuf.c, function m_cat():

    void
    m_cat(m, n)
	register struct mbuf *m, *n;
    {
	/*
	 * If there's no room, realloc
	 */
	if (M_FREEROOM(m) < n->m_len)
		m_inc(m,m->m_size+MINCSIZE);
	
	memcpy(m->m_data+m->m_len, n->m_data, n->m_len);  <<<< this memcpy 
							  corrupts the malloc
							  heap
        ....
    }


The problem is apparently in m_inc(), when an mbuf with an external
data buffer is resized.  After resizing the mbuf, the m_data member
still points into the old buffer, before is was reallocated.  For some
reason, the code to re-adjust the m_data pointer is present in the M_EXT
case in m_inc(), but is commented out. (With a bit of luck, realloc
might be able to adjust the size of the memory block in place; but
slirp shouldn't rely on this)

Fix: Adjust mbuf->m_data after a realloc of the external data buffer

% gdiff -u qemu-cvs/slirp/mbuf.c qemu-debug/slirp/mbuf.c
--- qemu-cvs/slirp/mbuf.c       2004-04-22 02:10:47.000000000 +0200
+++ qemu-debug/slirp/mbuf.c     2005-05-16 11:24:14.557567000 +0200
@@ -146,18 +146,19 @@
         struct mbuf *m;
         int size;
 {
+       int datasize;
+
        /* some compiles throw up on gotos.  This one we can fake. */
         if(m->m_size>size) return;

         if (m->m_flags & M_EXT) {
-         /* datasize = m->m_data - m->m_ext; */
+         datasize = m->m_data - m->m_ext;
          m->m_ext = (char *)realloc(m->m_ext,size);
 /*             if (m->m_ext == NULL)
  *                     return (struct mbuf *)NULL;
  */
-         /* m->m_data = m->m_ext + datasize; */
+         m->m_data = m->m_ext + datasize;
         } else {
-         int datasize;
          char *dat;
          datasize = m->m_data - m->m_dat;
          dat = (char *)malloc(size);

Re: [Qemu-devel] [PATCH] Fix for a malloc heap corruption problem in the slirp network code

[Fabrice Bellard]

Juergen Keil wrote:
> Compiling inside a NetBSD 1.5 qemu guest OS (source files are located
> on an NFS filesystem mounted from the Solaris host OS) crashes qemu
> with a malloc heap corruption error, when the slirp user mode
> networking code is in use.
> [...]

> Using the "electric fence" memory allocator, the location of the data
> corruption can be narrowed down to the destination address in the memcpy
> call in slirp/mbuf.c, function m_cat():
> 
>     void
>     m_cat(m, n)
> 	register struct mbuf *m, *n;
>     {
> 	/*
> 	 * If there's no room, realloc
> 	 */
> 	if (M_FREEROOM(m) < n->m_len)
> 		m_inc(m,m->m_size+MINCSIZE);

First this code is incorrect : it increases the size by MINCSIZE which 
can be smaller than the required size.

> 	
> 	memcpy(m->m_data+m->m_len, n->m_data, n->m_len);  <<<< this memcpy 
> 							  corrupts the malloc
> 							  heap
>         ....
>     }
> 
> 
> The problem is apparently in m_inc(), when an mbuf with an external
> data buffer is resized.  After resizing the mbuf, the m_data member
> still points into the old buffer, before is was reallocated.  For some
> reason, the code to re-adjust the m_data pointer is present in the M_EXT
> case in m_inc(), but is commented out. (With a bit of luck, realloc
> might be able to adjust the size of the memory block in place; but
> slirp shouldn't rely on this)
> 
> Fix: Adjust mbuf->m_data after a realloc of the external data buffer

OK. Does someone knows why this code was suppressed ?

Fabrice.

Re: [Qemu-devel] [PATCH] Fix for a malloc heap corruption problem in the slirp network code

[Juergen Keil]

> Fabrice Bellard wrote
> 
> Juergen Keil wrote:
> > Compiling inside a NetBSD 1.5 qemu guest OS (source files are located
> > on an NFS filesystem mounted from the Solaris host OS) crashes qemu
> > with a malloc heap corruption error, when the slirp user mode
> > networking code is in use.
> > [...]
> 
> > Using the "electric fence" memory allocator, the location of the data
> > corruption can be narrowed down to the destination address in the memcpy
> > call in slirp/mbuf.c, function m_cat():
> > 
> >     void
> >     m_cat(m, n)
> > 	register struct mbuf *m, *n;
> >     {
> > 	/*
> > 	 * If there's no room, realloc
> > 	 */
> > 	if (M_FREEROOM(m) < n->m_len)
> > 		m_inc(m,m->m_size+MINCSIZE);
> 
> First this code is incorrect : it increases the size by MINCSIZE which 
> can be smaller than the required size.

Yep.

The m_cat() code might work in the slirp environment because its only
use is to reassemble ip fragments, which have a max size (~1500 bytes) that
is always smaller than MINCSIZE (4096).

To make the code more robust, it won't hurt to make sure the size of the
free room on the "m" mbuf is at least "n->m_len" after the call to m_inc(),
for all sizes of "n->m_len".