[Qemu-devel] [PULL 0/1] slirp updates

[Qemu-devel] [PULL 0/1] slirp updates

[Samuel Thibault]

warning: redirection vers https://people.debian.org/~sthibault/qemu.git/
The following changes since commit 04d74e07b4542aad5aa4ad03951b38b767f5314a:

  slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to 04d74e07b4542aad5aa4ad03951b38b767f5314a:

  slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)

----------------------------------------------------------------
slirp updates

----------------------------------------------------------------

[Qemu-devel] [PULL 1/1] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

The if_fastq and if_batchq contain not only packets, but queues of packets
for the same socket. When sofree frees a socket, it thus has to clear ifq_so
from all the packets from the queues, not only the first.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/socket.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index ecec0295a9..cb7b5b608d 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -59,6 +59,27 @@ socreate(Slirp *slirp)
   return(so);
 }
 
+/*
+ * Remove references to so from the given message queue.
+ */
+static void
+soqfree(struct socket *so, struct quehead *qh)
+{
+    struct mbuf *ifq;
+
+    for (ifq = (struct mbuf *) qh->qh_link;
+             (struct quehead *) ifq != qh;
+             ifq = ifq->ifq_next) {
+        if (ifq->ifq_so == so) {
+            struct mbuf *ifm;
+            ifq->ifq_so = NULL;
+            for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+                ifm->ifq_so = NULL;
+            }
+        }
+    }
+}
+
 /*
  * remque and free a socket, clobber cache
  */
@@ -66,23 +87,9 @@ void
 sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
-  struct mbuf *ifm;
 
-  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
-       (struct quehead *) ifm != &slirp->if_fastq;
-       ifm = ifm->ifq_next) {
-    if (ifm->ifq_so == so) {
-      ifm->ifq_so = NULL;
-    }
-  }
-
-  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
-       (struct quehead *) ifm != &slirp->if_batchq;
-       ifm = ifm->ifq_next) {
-    if (ifm->ifq_so == so) {
-      ifm->ifq_so = NULL;
-    }
-  }
+  soqfree(so, &slirp->if_fastq);
+  soqfree(so, &slirp->if_batchq);
 
   if (so->so_emu==EMU_RSH && so->extra) {
 	sofree(so->extra);
-- 
2.14.1

Re: [Qemu-devel] [PULL 1/1] slirp: fix clearing ifq_so from pending packets

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On 08/27/2017 06:05 PM, Samuel Thibault wrote:
> The if_fastq and if_batchq contain not only packets, but queues of packets
> for the same socket. When sofree frees a socket, it thus has to clear ifq_so
> from all the packets from the queues, not only the first.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/socket.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)

This is a pull request, but missed 2.10-rc4, which makes it really hard
to justify for inclusion in 2.10 proper.  Is it okay that this doesn't
go in until 2.11? I've added qemu-stable, so it can also be queued for
2.10.1.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PULL 1/1] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Hello,

Eric Blake, on lun. 28 août 2017 10:43:48 -0500, wrote:
> This is a pull request, but missed 2.10-rc4, which makes it really hard
> to justify for inclusion in 2.10 proper.  Is it okay that this doesn't
> go in until 2.11?

Well, AIUI we have been living with the issue for years (decades?), so
it can probably wait a bit more :)

Samuel

Re: [Qemu-devel] [PULL 1/1] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Samuel Thibault, on lun. 28 août 2017 17:44:55 +0200, wrote:
> Eric Blake, on lun. 28 août 2017 10:43:48 -0500, wrote:
> > This is a pull request, but missed 2.10-rc4, which makes it really hard
> > to justify for inclusion in 2.10 proper.  Is it okay that this doesn't
> > go in until 2.11?
> 
> Well, AIUI we have been living with the issue for years (decades?), so
> it can probably wait a bit more :)

(OTOH, it's a at least DOS issue, userland can crash qemu with
specially-crafted packets).

Samuel

Re: [Qemu-devel] [PULL 0/1] slirp updates

[Peter Maydell]

On 28 August 2017 at 00:05, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> warning: redirection vers https://people.debian.org/~sthibault/qemu.git/
> The following changes since commit 04d74e07b4542aad5aa4ad03951b38b767f5314a:
>
>   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
>
> are available in the git repository at:
>
>   http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
>
> for you to fetch changes up to 04d74e07b4542aad5aa4ad03951b38b767f5314a:
>
>   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
>
> ----------------------------------------------------------------
> slirp updates
>
> ----------------------------------------------------------------

Is this pull request intended to be for 2.10 (in which
case it needs justification) or for 2.11 (in which case
it's a bit early) ?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/1] slirp updates

[Samuel Thibault]

Peter Maydell, on mar. 29 août 2017 10:20:31 +0100, wrote:
> On 28 August 2017 at 00:05, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> > warning: redirection vers https://people.debian.org/~sthibault/qemu.git/
> > The following changes since commit 04d74e07b4542aad5aa4ad03951b38b767f5314a:
> >
> >   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
> >
> > are available in the git repository at:
> >
> >   http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
> >
> > for you to fetch changes up to 04d74e07b4542aad5aa4ad03951b38b767f5314a:
> >
> >   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
> >
> > ----------------------------------------------------------------
> > slirp updates
> >
> > ----------------------------------------------------------------
> 
> Is this pull request intended to be for 2.10 (in which
> case it needs justification) or for 2.11 (in which case
> it's a bit early) ?

It is for 2.10. It fixes at least a DOS: userland can at least crash
qemu with specially-crafted packets.

Samuel

Re: [Qemu-devel] [PULL 0/1] slirp updates

[Peter Maydell]

On 29 August 2017 at 10:22, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Peter Maydell, on mar. 29 août 2017 10:20:31 +0100, wrote:
>> On 28 August 2017 at 00:05, Samuel Thibault
>> <samuel.thibault@ens-lyon.org> wrote:
>> > warning: redirection vers https://people.debian.org/~sthibault/qemu.git/
>> > The following changes since commit 04d74e07b4542aad5aa4ad03951b38b767f5314a:
>> >
>> >   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
>> >
>> > are available in the git repository at:
>> >
>> >   http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
>> >
>> > for you to fetch changes up to 04d74e07b4542aad5aa4ad03951b38b767f5314a:
>> >
>> >   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
>> >
>> > ----------------------------------------------------------------
>> > slirp updates
>> >
>> > ----------------------------------------------------------------
>>
>> Is this pull request intended to be for 2.10 (in which
>> case it needs justification) or for 2.11 (in which case
>> it's a bit early) ?
>
> It is for 2.10. It fixes at least a DOS: userland can at least crash
> qemu with specially-crafted packets.

It's really really hard to justify putting this in now.
If it had come in last Wednesday it would have been
easy to put into rc4. If it had come in next Thursday
we'd have shrugged and said "ok, needs to go into next
stable release". Right now it's a choice of "treat it
as if it had arrived in two days time" (ie don't put it
in 2.10) or delay the release by yet another rc cycle.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/1] slirp updates

[Samuel Thibault]

Peter Maydell, on mar. 29 août 2017 16:31:03 +0100, wrote:
> On 29 August 2017 at 10:22, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Peter Maydell, on mar. 29 août 2017 10:20:31 +0100, wrote:
> >> On 28 August 2017 at 00:05, Samuel Thibault
> >> <samuel.thibault@ens-lyon.org> wrote:
> >> > warning: redirection vers https://people.debian.org/~sthibault/qemu.git/
> >> > The following changes since commit 04d74e07b4542aad5aa4ad03951b38b767f5314a:
> >> >
> >> >   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
> >> >
> >> > are available in the git repository at:
> >> >
> >> >   http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
> >> >
> >> > for you to fetch changes up to 04d74e07b4542aad5aa4ad03951b38b767f5314a:
> >> >
> >> >   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
> >> >
> >> > ----------------------------------------------------------------
> >> > slirp updates
> >> >
> >> > ----------------------------------------------------------------
> >>
> >> Is this pull request intended to be for 2.10 (in which
> >> case it needs justification) or for 2.11 (in which case
> >> it's a bit early) ?
> >
> > It is for 2.10. It fixes at least a DOS: userland can at least crash
> > qemu with specially-crafted packets.
> 
> It's really really hard to justify putting this in now.

Alright, it's up to the release managers to decide anyway.

Samuel

Re: [Qemu-devel] [PULL 0/1] slirp updates

[Peter Maydell]

On 28 August 2017 at 00:05, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> warning: redirection vers https://people.debian.org/~sthibault/qemu.git/
> The following changes since commit 04d74e07b4542aad5aa4ad03951b38b767f5314a:
>
>   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
>
> are available in the git repository at:
>
>   http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
>
> for you to fetch changes up to 04d74e07b4542aad5aa4ad03951b38b767f5314a:
>
>   slirp: fix clearing ifq_so from pending packets (2017-08-26 01:04:12 +0200)
>
> ----------------------------------------------------------------
> slirp updates
>
> ----------------------------------------------------------------

Applied to master, thanks (I cherry-picked the patch so I
could add the cc:stable note).

-- PMM