Re: [Qemu-devel] [PATCH for-3.0] slirp: Correct size check in m_inc()

[Daniel P . Berrangé]

On Tue, Aug 07, 2018 at 01:52:24PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Maydell (address@hidden) wrote:
> > The data in an mbuf buffer is not necessarily at the start of the
> > allocated buffer. (For instance m_adj() allows data to be trimmed
> > from the start by just advancing the pointer and reducing the length.)
> > This means that the allocated buffer size (m->m_size) and the
> > amount of space from the m_data pointer to the end of the
> > buffer (M_ROOM(m)) are not necessarily the same.
> > 
> > Commit 864036e251f54c9 tried to change the m_inc() function from
> > taking the new allocated-buffer-size to taking the new room-size,
> > but forgot to change the initial "do we already have enough space"
> > check. This meant that if we were trying to extend a buffer which
> > had a leading gap between the buffer start and the data, we might
> > incorrectly decide it didn't need to be extended, and then
> > overrun the end of the buffer, causing memory corruption and
> > an eventual crash.
> > 
> > Change the "already big enough?" condition from checking the
> > argument against m->m_size to checking against M_ROOM().
> > This only makes a difference for the callsite in m_cat();
> > the other three callsites all start with a freshly allocated
> > mbuf from m_get(), which will have m->m_size == M_ROOM(m).
> > 
> > Fixes: 864036e251f54c9

IIUC, this changeset was a security fix for CVE-2018-11806.

Given that the fix was flawed and allowed guest to crash the host
with a new buffer overrun, it seems we need to get a new CVE allocated
too.

> > Fixes: https://bugs.launchpad.net/qemu/+bug/1785670
> > Signed-off-by: Peter Maydell <address@hidden>
> 
> Tested-by: Dr. David Alan Gilbert <address@hidden>
> 
> > ---
> >  slirp/mbuf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> > index 0c189e1a7bf..1b7868355a3 100644
> > --- a/slirp/mbuf.c
> > +++ b/slirp/mbuf.c
> > @@ -154,7 +154,7 @@ m_inc(struct mbuf *m, int size)
> >      int datasize;
> >  
> >      /* some compilers throw up on gotos.  This one we can fake. */
> > -    if (m->m_size > size) {
> > +    if (M_ROOM(m) > size) {
> >          return;
> >      }
> >  
> > -- 
> > 2.17.1
> > 
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|