| Message ID | 20180807114501.12370-1-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [for-3.0] slirp: Correct size check in m_inc() | expand |
Peter Maydell, le mar. 07 août 2018 12:45:01 +0100, a ecrit: > 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 > Fixes: https://bugs.launchpad.net/qemu/+bug/1785670 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > 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 > > -- Samuel "And the next time you consider complaining that running Lucid Emacs 19.05 via NFS from a remote Linux machine in Paraguay doesn't seem to get the background colors right, you'll know who to thank." (By Matt Welsh)
* 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 > Fixes: https://bugs.launchpad.net/qemu/+bug/1785670 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > 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 / dgilbert@redhat.com / Manchester, UK
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. > > Fixes: https://bugs.launchpad.net/qemu/+bug/1785670 > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > 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 / dgilbert@redhat.com / 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 :|
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. Thomas
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 :|
On 7 August 2018 at 12:45, 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 > Fixes: https://bugs.launchpad.net/qemu/+bug/1785670 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- Applied to master, thanks. -- PMM
On 7 August 2018 at 14:09, Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Aug 07, 2018 at 03:07:07PM +0200, Thomas Huth wrote: >> 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. Yeah, we haven't released anything with 864036e251f54c9 in it yet. (In particular we did not flag it up for stable and so it is not in 2.12.1...) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 7 August 2018 at 14:09, Daniel P. Berrangé <berrange@redhat.com> wrote: >> On Tue, Aug 07, 2018 at 03:07:07PM +0200, Thomas Huth wrote: >>> 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. > > Yeah, we haven't released anything with 864036e251f54c9 in it yet. > (In particular we did not flag it up for stable and so it is not > in 2.12.1...) Pointing out the obvious: this is a second opportunity to flag the CVE fix for stable.
On 7 August 2018 at 16:47, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 7 August 2018 at 14:09, Daniel P. Berrangé <berrange@redhat.com> wrote: >>> On Tue, Aug 07, 2018 at 03:07:07PM +0200, Thomas Huth wrote: >>>> 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. >> >> Yeah, we haven't released anything with 864036e251f54c9 in it yet. >> (In particular we did not flag it up for stable and so it is not >> in 2.12.1...) > > Pointing out the obvious: this is a second opportunity to flag the CVE > fix for stable. Well, as long as we get both halves of it, yes :-) (ie commits 864036e251f54c9 + 09b94ac0f29db3b022a77a). thanks -- PMM
* 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 > Fixes: https://bugs.launchpad.net/qemu/+bug/1785670 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > 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; > } I'm worried about a side effect of this change. A few lines below we have: datasize = m->m_data - m->m_dat; m->m_ext = g_malloc(size + datasize); memcpy(m->m_ext, m->m_dat, m->m_size); m->m_flags |= M_EXT; Question: What guarantees there's m_size room for that memcpy in the new m_ext? Before this fix, it used to be the case that m_size was smaller than size, so we knew it fitted; but that's no longer true. I don't think I understand the relationship between datasize and m_size enough to know if anything is sufficient. Dave > > -- > 2.17.1 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 9 August 2018 at 12:12, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> 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; >> } > > I'm worried about a side effect of this change. > A few lines below we have: > > datasize = m->m_data - m->m_dat; > m->m_ext = g_malloc(size + datasize); > memcpy(m->m_ext, m->m_dat, m->m_size); > m->m_flags |= M_EXT; > > Question: What guarantees there's m_size room for that > memcpy in the new m_ext? It did take me a while to convince myself that that was true when I was writing the patch... Here's the ASCII art: |--datasize---->|---m_len-------> |----------m_size------------------------------> |----M_ROOM--------------------> |-M_FREEROOM--> ^ ^ ^ m_dat m_data end of buffer ("datasize" is a bit misnamed, as it's "size of the leading gap between the start of the buffer and the data"; "gapsize" would be more helpful.) Anyway, we allocate size + datasize, and m_size == datasize + M_ROOM. We know that size >= M_ROOM, so the allocated buffer must be at least m_size big. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 9 August 2018 at 12:12, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > >> 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; > >> } > > > > I'm worried about a side effect of this change. > > A few lines below we have: > > > > datasize = m->m_data - m->m_dat; > > m->m_ext = g_malloc(size + datasize); > > memcpy(m->m_ext, m->m_dat, m->m_size); > > m->m_flags |= M_EXT; > > > > Question: What guarantees there's m_size room for that > > memcpy in the new m_ext? > > It did take me a while to convince myself that that was true > when I was writing the patch... Here's the ASCII art: > > > |--datasize---->|---m_len-------> > |----------m_size------------------------------> > |----M_ROOM--------------------> > |-M_FREEROOM--> > > ^ ^ ^ > m_dat m_data end of buffer > > ("datasize" is a bit misnamed, as it's "size of the leading > gap between the start of the buffer and the data"; "gapsize" > would be more helpful.) > > Anyway, we allocate size + datasize, and > m_size == datasize + M_ROOM. We know that size >= M_ROOM, > so the allocated buffer must be at least m_size big. Ah OK, thanks. (That ascii art could do with being in a comment somewhere!) Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert, le jeu. 09 août 2018 12:32:05 +0100, a ecrit: > > |--datasize---->|---m_len-------> > > |----------m_size------------------------------> > > |----M_ROOM--------------------> > > |-M_FREEROOM--> > > > > ^ ^ ^ > > m_dat m_data end of buffer > > > > ("datasize" is a bit misnamed, as it's "size of the leading > > gap between the start of the buffer and the data"; "gapsize" > > would be more helpful.) > > > > Anyway, we allocate size + datasize, and > > m_size == datasize + M_ROOM. We know that size >= M_ROOM, > > so the allocated buffer must be at least m_size big. > > Ah OK, thanks. > (That ascii art could do with being in a comment somewhere!) Indeed. Peter, maybe your Signed-off-by on this? :) Samuel commit 4be85a1eeb6b19e91491e689d4d0d054030cbb49 Author: Peter Maydell <peter.maydell@linaro.org> Date: Thu Aug 9 23:52:59 2018 +0200 slirp: document mbuf pointers and sizes Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> diff --git a/slirp/mbuf.h b/slirp/mbuf.h index 33b84485d6..a5bb3f9e66 100644 --- a/slirp/mbuf.h +++ b/slirp/mbuf.h @@ -47,6 +47,16 @@ * free the m_ext. This is inefficient memory-wise, but who cares. */ +/* + * |--gapsize----->|---m_len-------> + * |----------m_size------------------------------> + * |----M_ROOM--------------------> + * |-M_FREEROOM--> + * + * ^ ^ ^ + * m_dat/m_ext m_data end of buffer + */ + /* * How much room is in the mbuf, from m_data to the end of the mbuf */
On 9 August 2018 at 22:54, Samuel Thibault <samuel.thibault@gnu.org> wrote: > Dr. David Alan Gilbert, le jeu. 09 août 2018 12:32:05 +0100, a ecrit: >> > |--datasize---->|---m_len-------> >> > |----------m_size------------------------------> >> > |----M_ROOM--------------------> >> > |-M_FREEROOM--> >> > >> > ^ ^ ^ >> > m_dat m_data end of buffer >> > >> > ("datasize" is a bit misnamed, as it's "size of the leading >> > gap between the start of the buffer and the data"; "gapsize" >> > would be more helpful.) >> > >> > Anyway, we allocate size + datasize, and >> > m_size == datasize + M_ROOM. We know that size >= M_ROOM, >> > so the allocated buffer must be at least m_size big. >> >> Ah OK, thanks. >> (That ascii art could do with being in a comment somewhere!) > > Indeed. Peter, maybe your Signed-off-by on this? :) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Samuel > > commit 4be85a1eeb6b19e91491e689d4d0d054030cbb49 > Author: Peter Maydell <peter.maydell@linaro.org> > Date: Thu Aug 9 23:52:59 2018 +0200 > > slirp: document mbuf pointers and sizes > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > diff --git a/slirp/mbuf.h b/slirp/mbuf.h > index 33b84485d6..a5bb3f9e66 100644 > --- a/slirp/mbuf.h > +++ b/slirp/mbuf.h > @@ -47,6 +47,16 @@ > * free the m_ext. This is inefficient memory-wise, but who cares. > */ > > +/* > + * |--gapsize----->|---m_len-------> > + * |----------m_size------------------------------> > + * |----M_ROOM--------------------> > + * |-M_FREEROOM--> > + * > + * ^ ^ ^ > + * m_dat/m_ext m_data end of buffer > + */ > + ...but (a) you should add a comment describing what 'gapsize' is, ie that there may be a gap between the in-use data and the start of the allocated buffer, and (b) m_inc() should change its variable name to match. thanks -- PMM
Peter Maydell, le ven. 10 août 2018 10:02:37 +0100, a ecrit: > (a) you should add a comment describing what 'gapsize' > is, ie that there may be a gap between the in-use data and the > start of the allocated buffer, and > (b) m_inc() should change its variable name to match. Right, I have done so in my tree. Do you prefer to have it pushed for qemu 3.0? Samuel
On 10 August 2018 at 10:08, Samuel Thibault <samuel.thibault@gnu.org> wrote: > Peter Maydell, le ven. 10 août 2018 10:02:37 +0100, a ecrit: >> (a) you should add a comment describing what 'gapsize' >> is, ie that there may be a gap between the in-use data and the >> start of the allocated buffer, and >> (b) m_inc() should change its variable name to match. > > Right, I have done so in my tree. Do you prefer to have it pushed for > qemu 3.0? No, this is definitely 3.1 material (also you should post the patch for review as its own email, please). thanks -- PMM
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; }
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 Fixes: https://bugs.launchpad.net/qemu/+bug/1785670 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- slirp/mbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1