9ae3a8
From 1f177df6a47fb1e2961067a50e005efad52595cc Mon Sep 17 00:00:00 2001
9ae3a8
From: Ladi Prosek <lprosek@redhat.com>
9ae3a8
Date: Wed, 5 Oct 2016 17:22:26 +0200
9ae3a8
Subject: [PATCH 4/8] balloon: fix segfault and harden the stats queue
9ae3a8
9ae3a8
RH-Author: Ladi Prosek <lprosek@redhat.com>
9ae3a8
Message-id: <1475666548-9186-5-git-send-email-lprosek@redhat.com>
9ae3a8
Patchwork-id: 72483
9ae3a8
O-Subject: [RHEL-7.4 qemu-kvm v2 PATCH 4/6] balloon: fix segfault and harden the stats queue
9ae3a8
Bugzilla: 1377968
9ae3a8
RH-Acked-by: Paolo Bonzini <pbonzini@redhat.com>
9ae3a8
RH-Acked-by: Michael S. Tsirkin <mst@redhat.com>
9ae3a8
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
9ae3a8
9ae3a8
The segfault here is triggered by the driver notifying the stats queue
9ae3a8
twice after adding a buffer to it. This effectively resets stats_vq_elem
9ae3a8
back to NULL and QEMU crashes on the next stats timer tick in
9ae3a8
balloon_stats_poll_cb.
9ae3a8
9ae3a8
This is a regression introduced in 51b19ebe4320f3dc, although admittedly
9ae3a8
the device assumed too much about the stats queue protocol even before
9ae3a8
that commit. This commit adds a few more checks and ensures that the one
9ae3a8
stats buffer gets deallocated on device reset.
9ae3a8
9ae3a8
Cc: qemu-stable@nongnu.org
9ae3a8
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
9ae3a8
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
9ae3a8
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
9ae3a8
(cherry picked from commit 4eae2a657d1ff5ada56eb9b4966eae0eff333b0b)
9ae3a8
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
9ae3a8
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
9ae3a8
9ae3a8
Conflicts:
9ae3a8
  * 1.5.3 does not return pointers from virtqueue_pop so only the
9ae3a8
    "harden the stats queue" part of the upstream patch description
9ae3a8
    applies
9ae3a8
  * a new field stats_vq_elem_pending is introduced to keep track
9ae3a8
    of the state of stats_vq_elem in lieu of its nullness upstream
9ae3a8
  * virtio_balloon_device_reset only resets stats_vq_elem_pending
9ae3a8
    because there is nothing to free
9ae3a8
---
9ae3a8
 hw/virtio/virtio-balloon.c         | 27 +++++++++++++++++++++++----
9ae3a8
 include/hw/virtio/virtio-balloon.h |  1 +
9ae3a8
 2 files changed, 24 insertions(+), 4 deletions(-)
9ae3a8
9ae3a8
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
9ae3a8
index 016dc60..17b3029 100644
9ae3a8
--- a/hw/virtio/virtio-balloon.c
9ae3a8
+++ b/hw/virtio/virtio-balloon.c
9ae3a8
@@ -95,13 +95,14 @@ static void balloon_stats_poll_cb(void *opaque)
9ae3a8
     VirtIOBalloon *s = opaque;
9ae3a8
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
9ae3a8
 
9ae3a8
-    if (!balloon_stats_supported(s)) {
9ae3a8
+    if (!s->stats_vq_elem_pending || !balloon_stats_supported(s)) {
9ae3a8
         /* re-schedule */
9ae3a8
         balloon_stats_change_timer(s, s->stats_poll_interval);
9ae3a8
         return;
9ae3a8
     }
9ae3a8
 
9ae3a8
     virtqueue_push(s->svq, &s->stats_vq_elem, s->stats_vq_offset);
9ae3a8
+    s->stats_vq_elem_pending = false;
9ae3a8
     virtio_notify(vdev, s->svq);
9ae3a8
 }
9ae3a8
 
9ae3a8
@@ -220,14 +221,22 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
9ae3a8
 static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
9ae3a8
 {
9ae3a8
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
9ae3a8
-    VirtQueueElement *elem = &s->stats_vq_elem;
9ae3a8
+    VirtQueueElement elem;
9ae3a8
     VirtIOBalloonStat stat;
9ae3a8
     size_t offset = 0;
9ae3a8
     qemu_timeval tv;
9ae3a8
 
9ae3a8
-    if (!virtqueue_pop(vq, elem)) {
9ae3a8
+    if (!virtqueue_pop(vq, &elem)) {
9ae3a8
         goto out;
9ae3a8
     }
9ae3a8
+    if (s->stats_vq_elem_pending) {
9ae3a8
+        /* This should never happen if the driver follows the spec. */
9ae3a8
+        virtqueue_push(vq, &s->stats_vq_elem, 0);
9ae3a8
+        virtio_notify(vdev, vq);
9ae3a8
+    }
9ae3a8
+
9ae3a8
+    s->stats_vq_elem = elem;
9ae3a8
+    s->stats_vq_elem_pending = true;
9ae3a8
 
9ae3a8
     /* Initialize the stats to get rid of any stale values.  This is only
9ae3a8
      * needed to handle the case where a guest supports fewer stats than it
9ae3a8
@@ -235,7 +244,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
9ae3a8
      */
9ae3a8
     reset_stats(s);
9ae3a8
 
9ae3a8
-    while (iov_to_buf(elem->out_sg, elem->out_num, offset, &stat, sizeof(stat))
9ae3a8
+    while (iov_to_buf(elem.out_sg, elem.out_num, offset, &stat, sizeof(stat))
9ae3a8
            == sizeof(stat)) {
9ae3a8
         uint16_t tag = tswap16(stat.tag);
9ae3a8
         uint64_t val = tswap64(stat.val);
9ae3a8
@@ -384,6 +393,15 @@ static void virtio_balloon_device_exit(VirtIODevice *vdev)
9ae3a8
     virtio_cleanup(vdev);
9ae3a8
 }
9ae3a8
 
9ae3a8
+static void virtio_balloon_device_reset(VirtIODevice *vdev)
9ae3a8
+{
9ae3a8
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
9ae3a8
+
9ae3a8
+    if (s->stats_vq_elem_pending) {
9ae3a8
+        s->stats_vq_elem_pending = false;
9ae3a8
+    }
9ae3a8
+}
9ae3a8
+
9ae3a8
 static Property virtio_balloon_properties[] = {
9ae3a8
     DEFINE_PROP_END_OF_LIST(),
9ae3a8
 };
9ae3a8
@@ -396,6 +414,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
9ae3a8
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
9ae3a8
     vdc->init = virtio_balloon_device_init;
9ae3a8
     vdc->exit = virtio_balloon_device_exit;
9ae3a8
+    vdc->reset = virtio_balloon_device_reset;
9ae3a8
     vdc->get_config = virtio_balloon_get_config;
9ae3a8
     vdc->set_config = virtio_balloon_set_config;
9ae3a8
     vdc->get_features = virtio_balloon_get_features;
9ae3a8
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
9ae3a8
index f863bfe..a84736b 100644
9ae3a8
--- a/include/hw/virtio/virtio-balloon.h
9ae3a8
+++ b/include/hw/virtio/virtio-balloon.h
9ae3a8
@@ -63,6 +63,7 @@ typedef struct VirtIOBalloon {
9ae3a8
     uint32_t actual;
9ae3a8
     uint64_t stats[VIRTIO_BALLOON_S_NR];
9ae3a8
     VirtQueueElement stats_vq_elem;
9ae3a8
+    bool stats_vq_elem_pending;
9ae3a8
     size_t stats_vq_offset;
9ae3a8
     QEMUTimer *stats_timer;
9ae3a8
     int64_t stats_last_update;
9ae3a8
-- 
9ae3a8
1.8.3.1
9ae3a8