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

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

[Peter Maydell]

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;
     }
 
-- 
2.17.1

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

[Samuel Thibault]

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)

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

[Dr. David Alan Gilbert]

* 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

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

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

[Thomas Huth]

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

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

[Daniel P. Berrangé]

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

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

[Peter Maydell]

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

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

[Peter Maydell]

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

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

[Markus Armbruster]

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.

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

[Peter Maydell]

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

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

[Dr. David Alan Gilbert]

* 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

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

[Peter Maydell]

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

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

[Dr. David Alan Gilbert]

* 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

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

[Samuel Thibault]

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
  */

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

[Peter Maydell]

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

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

[Samuel Thibault]

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

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

[Peter Maydell]

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