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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Aug 07, 2018 at 03:07:07PM +0200, Thomas Huth wrote:
> On 08/07/2018 02:58 PM, Daniel P. Berrangé wrote:
> > On Tue, Aug 07, 2018 at 01:52:24PM +0100, Dr. David Alan Gilbert wrote:
> >> * Peter Maydell (peter.maydell@linaro.org) 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.
> 
> But 864036e251f54c9 was never part of an official QEMU release, was it?
> Or did it go into a stable release already? If not, I think you simply
> need both patches to fix the CVE instead.

Ah possibly - I  didn't look at where 864036e251f54c9 was actually
release or not. If its onyl git master, then yeah, we can use the
same CVE we already have.

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