Re: [Qemu-devel] [PULL 19/22] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Dr. David Alan Gilbert]

* Peter Maydell (address@hidden) wrote:
> On Wed, 6 Mar 2019 at 11:55, Dr. David Alan Gilbert (git)
> <address@hidden> wrote:
> >
> > From: Wei Wang <address@hidden>


Wei: Can you look at this....

> > The new feature enables the virtio-balloon device to receive hints of
> > guest free pages from the free page vq.
> >
> > A notifier is registered to the migration precopy notifier chain. The
> > notifier calls free_page_start after the migration thread syncs the dirty
> > bitmap, so that the free page optimization starts to clear bits of free
> > pages from the bitmap. It calls the free_page_stop before the migration
> > thread syncs the bitmap, which is the end of the current round of ram
> > save. The free_page_stop is also called to stop the optimization in the
> > case when there is an error occurred in the process of ram saving.
> >
> > Note: balloon will report pages which were free at the time of this call.
> > As the reporting happens asynchronously, dirty bit logging must be
> > enabled before this free_page_start call is made. Guest reporting must be
> > disabled before the migration dirty bitmap is synchronized.
> >
> > Signed-off-by: Wei Wang <address@hidden>
> > CC: Michael S. Tsirkin <address@hidden>
> > CC: Dr. David Alan Gilbert <address@hidden>
> > CC: Juan Quintela <address@hidden>
> > CC: Peter Xu <address@hidden>
> > Message-Id: <address@hidden>
> > Reviewed-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> >   dgilbert: Dropped kernel header update, fixed up CMD_ID_* name change
> > ---
> 
> Hi -- Coverity points out a use-after-free here (CID 1399412):
> 
> > +static bool get_free_page_hints(VirtIOBalloon *dev)
> > +{
> > +    VirtQueueElement *elem;
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +    VirtQueue *vq = dev->free_page_vq;
> > +
> > +    while (dev->block_iothread) {
> > +        qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
> > +    }
> > +
> > +    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > +    if (!elem) {
> > +        return false;
> > +    }
> > +
> > +    if (elem->out_num) {
> > +        uint32_t id;
> > +        size_t size = iov_to_buf(elem->out_sg, elem->out_num, 0,
> > +                                 &id, sizeof(id));
> > +        virtqueue_push(vq, elem, size);
> > +        g_free(elem);
> 
> Here we free elem...
> 
> > +
> > +        virtio_tswap32s(vdev, &id);
> > +        if (unlikely(size != sizeof(id))) {
> > +            virtio_error(vdev, "received an incorrect cmd id");
> > +            return false;
> > +        }
> > +        if (id == dev->free_page_report_cmd_id) {
> > +            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> > +        } else {
> > +            /*
> > +             * Stop the optimization only when it has started. This
> > +             * avoids a stale stop sign for the previous command.
> > +             */
> > +            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > +                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (elem->in_num) {
> 
> ...but we can fall through here and try to dereference elem...
> 
> > +        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
> > +            qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> > +                                      elem->in_sg[0].iov_len);
> > +        }
> > +        virtqueue_push(vq, elem, 1);
> > +        g_free(elem);
> 
> ...and then free it again.

OK, so the question here is:
  Is it allowed to have both in and out in the same queue element; if
it's not then we need to error the device.
  If it is allowed then we need to fix up the out_num case.

Dave

> > +    }
> > +
> > +    return true;
> > +}
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK