[Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Qemu-devel] [PATCH] 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>
Acked-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] [PATCH] slirp: fix clearing ifq_so from pending packets

[Philippe Mathieu-Daudé]

Hi Sam, thanks for this patch :)

On 08/25/2017 07:37 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>
> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

so let's raise this to:
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);
> 

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Hello,

So Wjjzhang and PJP, can you confirm that this fixes your uses?

Samuel

Samuel Thibault, on sam. 26 août 2017 00:37:21 +0200, 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>
> Acked-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
> 

-- 
Samuel
...
<rv_> et Ctrl alt F2 pour aller sous console
<rv_> mais c koi pour passer d'un bureau a un autre !
<rv_> au fait c koi le raccourci pour passer d'un bureau a un autre 'question stupide"
<cycyx> ça dépend du window manager et de ta conf
<Firebird> ce qui fonctionne toujours c'est CTRL-ALT-BCKSP
-:- SignOff rv_: #linuxfr (Read error: EOF from client)
-:- rv_ [~rv@217.11.166.169] has joined #linuxfr
<rv_> Firebird: MEURT...

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Samuel Thibault, on sam. 26 août 2017 01:05:04 +0200, wrote:
> So Wjjzhang and PJP, can you confirm that this fixes your uses?

PJP, can you forward it to Wjjzhang?  I keep getting

<wjjzhang@tencent.com>: host cloudmx.qq.com[113.108.11.188] said: 550 Mail
    content denied.
    http://ascloud.qq.com/cgi-bin/readtemplate?t=anti_spam_errors#n1000726 (in
    reply to end of DATA command)

Samuel

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[P J P]

  Hello Samuel,

+-- On Sat, 26 Aug 2017, Samuel Thibault wrote --+
| So Wjjzhang and PJP, can you confirm that this fixes your uses?

Yes, I confirm the patch fixes the use-after-free issue.

Thank you so much.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Thomas Huth]

 Hi Samuel,

On 26.08.2017 00:37, 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.

I think you should CC: this to qemu-stable if it's fixing a problem that
can be used by the guest to crash QEMU... ?

 Thomas

> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Acked-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);
> 

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Samuel Thibault]

Thomas Huth, on mer. 30 août 2017 09:50:45 +0200, wrote:
> On 26.08.2017 00:37, 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.
> 
> I think you should CC: this to qemu-stable if it's fixing a problem that
> can be used by the guest to crash QEMU... ?

Indeed. I thought it should first go to master.

Samuel

>  Thomas
> 
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > Acked-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);
> > 
> 

-- 
Samuel
 CN > J'ai enseigné l'algorythmique.
 GLG> C'est quoi l'algorythmique ? Une contrebasse programmée en Algol ?
 -+- in : Guide du Neuneu d'Usenet - Neuneu fait ses gammes. -+-

Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets

[Peter Maydell]

On 30 August 2017 at 08:52, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Thomas Huth, on mer. 30 août 2017 09:50:45 +0200, wrote:
>> On 26.08.2017 00:37, 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.
>>
>> I think you should CC: this to qemu-stable if it's fixing a problem that
>> can be used by the guest to crash QEMU... ?
>
> Indeed. I thought it should first go to master.

The way our cc-stable process works is that you include
a line in the commit message "Cc: qemu-stable@nongnu.org"
(in the same place as the signed-off-by and other lines)
and also cc the email address when sending the patch;
then that will go into the git history in master and can
be fished out from there. If you don't include it in the
original patch send then whoever puts the pull request
together has to include it, so it's helpful to add it from
the start if it's pretty definitely a thing that should go
into stable.

thanks
-- PMM