Re: [Qemu-devel] [PULL 05/15] multifd: Be flexible about packet size

[Peter Maydell]

On Tue, 26 Mar 2019 at 12:26, Peter Maydell <address@hidden> wrote:
>
> On Mon, 25 Mar 2019 at 18:13, Juan Quintela <address@hidden> wrote:
> >
> > This way we can change the packet size in the future and everything
> > will work.  We choose an arbitrary big number (100 times configured
> > size) as a limit about how big we will reallocate.
> >
> > Signed-off-by: Juan Quintela <address@hidden>
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > Signed-off-by: Juan Quintela <address@hidden>
> > --
>
> Hi; Coverity reports a use-after-free in this code
> (CID 1400442):

Ping? Have we got a fix for this yet ?

thanks
-- PMM

>
> > @@ -832,12 +832,24 @@ static int 
> > multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> >      p->flags = be32_to_cpu(packet->flags);
> >
> >      packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
> > -    if (packet->pages_alloc > page_count) {
> > +    /*
> > +     * If we recevied a packet that is 100 times bigger than expected
> > +     * just stop migration.  It is a magic number.
> > +     */
> > +    if (packet->pages_alloc > pages_max * 100) {
> >          error_setg(errp, "multifd: received packet "
> > -                   "with size %d and expected maximum size %d",
> > -                   packet->pages_alloc, page_count) ;
> > +                   "with size %d and expected a maximum size of %d",
> > +                   packet->pages_alloc, pages_max * 100) ;
> >          return -1;
> >      }
> > +    /*
> > +     * We received a packet that is bigger than expected but inside
> > +     * reasonable limits (see previous comment).  Just reallocate.
> > +     */
> > +    if (packet->pages_alloc > p->pages->allocated) {
> > +        multifd_pages_clear(p->pages);
>
> multifd_pages_clear() calls g_free() on the pointer it is passed...
>
> > +        multifd_pages_init(packet->pages_alloc);
> > +    }
> >
> >      p->pages->used = be32_to_cpu(packet->pages_used);
>
> ...but here we fall through and dereference p->pages, which
> we might have just freed.
>
> >      if (p->pages->used > packet->pages_alloc) {
> > --
> > 2.20.1
>
> thanks
> -- PMM