Re: [Qemu-stable] [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket

[mdroth]

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 <address@hidden>

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