Patchwork slirp: fixed potential use-after-free of a socket

login
register
mail settings
Submitter Vitaly Chipounov
Date Feb. 15, 2013, 11 a.m.
Message ID <1360926013-23725-1-git-send-email-vitaly.chipounov@epfl.ch>
Download mbox | patch
Permalink /patch/220719/
State New
Headers show

Comments

Vitaly Chipounov - Feb. 15, 2013, 11 a.m.
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.

Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>
---
 slirp/socket.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
Jan Kiszka - Feb. 21, 2013, 2:33 p.m.
On 2013-02-15 12:00, Vitaly Chipounov wrote:
> A socket may still have references to it in various queues
> at the time it is freed, causing memory corruptions.

Did you see it in practice? Or is this patch based on code review? What
will happen if those queued mbufs find their ifq_so NULL?

> 
> Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>
> ---
>  slirp/socket.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 77b0c98..8a7adc8 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -55,6 +55,33 @@ socreate(Slirp *slirp)
>    return(so);
>  }
>  
> +/**
> + * It may happen that a socket is still referenced in various
> + * mbufs at the time it is freed. Clear all references to the
> + * socket here.
> + */
> +static void soremovefromqueues(struct socket *so)
> +{
> +    if (!so->so_queued) {
> +        return;
> +    }
> +
> +    Slirp *slirp = so->slirp;
> +    struct mbuf *ifm;
> +
> +    for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) {

This line and the one below smells like checkpatch.pl wasn't run against
it. Granted, slirp patches easy make checkpatch upset as the input is
not properly formatted, but please don't add more violations.

> +        if (ifm->ifq_so == so) {
> +            ifm->ifq_so = NULL;
> +        }
> +    }
> +
> +    for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) {
> +        if (ifm->ifq_so == so) {
> +            ifm->ifq_so = NULL;
> +        }
> +    }
> +}
> +
>  /*
>   * remque and free a socket, clobber cache
>   */
> @@ -79,6 +106,8 @@ sofree(struct socket *so)
>    if(so->so_next && so->so_prev)
>      remque(so);  /* crashes if so is not in a queue */
>  
> +  soremovefromqueues(so);
> +
>    free(so);
>  }
>  
> 

Sorry for the late feedback,
Jan
Michael Roth - Feb. 21, 2013, 9:47 p.m.
On Fri, Feb 15, 2013 at 12:00:13PM +0100, Vitaly Chipounov wrote:
> A socket may still have references to it in various queues
> at the time it is freed, causing memory corruptions.
> 
> Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>
> ---
>  slirp/socket.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 77b0c98..8a7adc8 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -55,6 +55,33 @@ socreate(Slirp *slirp)
>    return(so);
>  }
> 
> +/**
> + * It may happen that a socket is still referenced in various
> + * mbufs at the time it is freed. Clear all references to the
> + * socket here.
> + */
> +static void soremovefromqueues(struct socket *so)
> +{
> +    if (!so->so_queued) {
> +        return;
> +    }
> +
> +    Slirp *slirp = so->slirp;
> +    struct mbuf *ifm;

Declarations should come at the beginning of a block/function (see: ...
er, I could've sworn it was in HACKING or CODING_STYLE but maybe not.
That's the standard in any case)

> +
> +    for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) {
> +        if (ifm->ifq_so == so) {
> +            ifm->ifq_so = NULL;
> +        }
> +    }
> +
> +    for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) {
> +        if (ifm->ifq_so == so) {
> +            ifm->ifq_so = NULL;
> +        }
> +    }

Is the intention just to mark them null or actually remove? Not sure
which, but either the logic should change or the function name should
(soinvalidate() or something along that line if the former).

> +}
> +
>  /*
>   * remque and free a socket, clobber cache
>   */
> @@ -79,6 +106,8 @@ sofree(struct socket *so)
>    if(so->so_next && so->so_prev)
>      remque(so);  /* crashes if so is not in a queue */
> 
> +  soremovefromqueues(so);
> +
>    free(so);
>  }
> 
> -- 
> 1.7.10
> 
>
Michael Roth - Feb. 21, 2013, 10:03 p.m.
On Thu, Feb 21, 2013 at 03:47:25PM -0600, mdroth wrote:
> On Fri, Feb 15, 2013 at 12:00:13PM +0100, Vitaly Chipounov wrote:
> > A socket may still have references to it in various queues
> > at the time it is freed, causing memory corruptions.
> > 
> > Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>

Meant to cc qemu-stable

> > ---
> >  slirp/socket.c |   29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/slirp/socket.c b/slirp/socket.c
> > index 77b0c98..8a7adc8 100644
> > --- a/slirp/socket.c
> > +++ b/slirp/socket.c
> > @@ -55,6 +55,33 @@ socreate(Slirp *slirp)
> >    return(so);
> >  }
> > 
> > +/**
> > + * It may happen that a socket is still referenced in various
> > + * mbufs at the time it is freed. Clear all references to the
> > + * socket here.
> > + */
> > +static void soremovefromqueues(struct socket *so)
> > +{
> > +    if (!so->so_queued) {
> > +        return;
> > +    }
> > +
> > +    Slirp *slirp = so->slirp;
> > +    struct mbuf *ifm;
> 
> Declarations should come at the beginning of a block/function (see: ...
> er, I could've sworn it was in HACKING or CODING_STYLE but maybe not.
> That's the standard in any case)
> 
> > +
> > +    for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) {
> > +        if (ifm->ifq_so == so) {
> > +            ifm->ifq_so = NULL;
> > +        }
> > +    }
> > +
> > +    for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) {
> > +        if (ifm->ifq_so == so) {
> > +            ifm->ifq_so = NULL;
> > +        }
> > +    }
> 
> Is the intention just to mark them null or actually remove? Not sure
> which, but either the logic should change or the function name should
> (soinvalidate() or something along that line if the former).
> 
> > +}
> > +
> >  /*
> >   * remque and free a socket, clobber cache
> >   */
> > @@ -79,6 +106,8 @@ sofree(struct socket *so)
> >    if(so->so_next && so->so_prev)
> >      remque(so);  /* crashes if so is not in a queue */
> > 
> > +  soremovefromqueues(so);
> > +
> >    free(so);
> >  }
> > 
> > -- 
> > 1.7.10
> > 
> >
Vitaly Chipounov - Feb. 22, 2013, 9:57 a.m.
Hi,

On 21.02.2013 15:33, Jan Kiszka wrote:
> On 2013-02-15 12:00, Vitaly Chipounov wrote:
>> A socket may still have references to it in various queues
>> at the time it is freed, causing memory corruptions.
> Did you see it in practice? Or is this patch based on code review? What
> will happen if those queued mbufs find their ifq_so NULL?

I have a packet trace that triggers this problem when it is injected
into the guest's NIC.
I am still not quite sure why this happens, but suspect that it could be
caused by malformed/partial TCP/IP streams.

Setting ifq_so to NULL shouldn't be an issue because there seems to be
checks for that (e.g., in if.c).
It doesn't seem to affect normal web browsing/downloading in the guest.

Vitaly

>
>> Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>
>> ---
>>  slirp/socket.c |   29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/slirp/socket.c b/slirp/socket.c
>> index 77b0c98..8a7adc8 100644
>> --- a/slirp/socket.c
>> +++ b/slirp/socket.c
>> @@ -55,6 +55,33 @@ socreate(Slirp *slirp)
>>    return(so);
>>  }
>>  
>> +/**
>> + * It may happen that a socket is still referenced in various
>> + * mbufs at the time it is freed. Clear all references to the
>> + * socket here.
>> + */
>> +static void soremovefromqueues(struct socket *so)
>> +{
>> +    if (!so->so_queued) {
>> +        return;
>> +    }
>> +
>> +    Slirp *slirp = so->slirp;
>> +    struct mbuf *ifm;
>> +
>> +    for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) {
> This line and the one below smells like checkpatch.pl wasn't run against
> it. Granted, slirp patches easy make checkpatch upset as the input is
> not properly formatted, but please don't add more violations.
>
>> +        if (ifm->ifq_so == so) {
>> +            ifm->ifq_so = NULL;
>> +        }
>> +    }
>> +
>> +    for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) {
>> +        if (ifm->ifq_so == so) {
>> +            ifm->ifq_so = NULL;
>> +        }
>> +    }
>> +}
>> +
>>  /*
>>   * remque and free a socket, clobber cache
>>   */
>> @@ -79,6 +106,8 @@ sofree(struct socket *so)
>>    if(so->so_next && so->so_prev)
>>      remque(so);  /* crashes if so is not in a queue */
>>  
>> +  soremovefromqueues(so);
>> +
>>    free(so);
>>  }
>>  
>>
> Sorry for the late feedback,
> Jan
>
Jan Kiszka - Feb. 22, 2013, 10:06 a.m.
On 2013-02-22 10:57, Vitaly Chipounov wrote:
> Hi,
> 
> On 21.02.2013 15:33, Jan Kiszka wrote:
>> On 2013-02-15 12:00, Vitaly Chipounov wrote:
>>> A socket may still have references to it in various queues
>>> at the time it is freed, causing memory corruptions.
>> Did you see it in practice? Or is this patch based on code review? What
>> will happen if those queued mbufs find their ifq_so NULL?
> 
> I have a packet trace that triggers this problem when it is injected
> into the guest's NIC.
> I am still not quite sure why this happens, but suspect that it could be
> caused by malformed/partial TCP/IP streams.

OK. Is it shareable?

We need to understand why these bufs still hang around - and exclude
they leak or cause more problems elsewhere.

Jan

Patch

diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@  socreate(Slirp *slirp)
   return(so);
 }
 
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+    if (!so->so_queued) {
+        return;
+    }
+
+    Slirp *slirp = so->slirp;
+    struct mbuf *ifm;
+
+    for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) {
+        if (ifm->ifq_so == so) {
+            ifm->ifq_so = NULL;
+        }
+    }
+
+    for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) {
+        if (ifm->ifq_so == so) {
+            ifm->ifq_so = NULL;
+        }
+    }
+}
+
 /*
  * remque and free a socket, clobber cache
  */
@@ -79,6 +106,8 @@  sofree(struct socket *so)
   if(so->so_next && so->so_prev)
     remque(so);  /* crashes if so is not in a queue */
 
+  soremovefromqueues(so);
+
   free(so);
 }