On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin [off-list ref] wrote:

On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:

quoted

On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek [off-list ref] wrote:

quoted

On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand [off-list ref] wrote:

quoted

On 23.03.2017 16:17, Arnd Bergmann wrote:

quoted

The latest gcc-7.0.1 snapshot reports a new warning:

virtio/virtio_balloon.c: In function 'update_balloon_stats':
virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]

This seems absolutely right, so we should add an extra check to
prevent copying uninitialized stack data into the statistics.
From all I can tell, this has been broken since the statistics code
was originally added in 2.6.34.

Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e1191508228..cd5c54e2003d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c

@@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)

      available = si_mem_available();

+#ifdef CONFIG_VM_EVENT_COUNTERS
      update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
                              pages_to_bytes(events[PSWPIN]));
      update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
                              pages_to_bytes(events[PSWPOUT]));
      update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
      update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#endif
      update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
                              pages_to_bytes(i.freeram));
      update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

This will leave four uninitialized slots in vb->stats if
CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
have

  BUG_ON(idx < VIRTIO_BALLOON_S_NR);

at the end.

You need to make sure that vb->stats is smaller, either by using
something else than VIRTIO_BALLOON_S_NR for its size or something else
than sizeof(vb->stats) as the last argument to sg_init_one in this
file.

Ah, got it. Would one of you create a fixed patch for this, or should I?

I will do it. If we go the route of returning 'idx' from
update_balloon_stats, my patch

   virtio_balloon: don't push uninitialized buffers to stats virtqueue

needs to be redone anyway. Thanks!

quoted

An easy way to solve it would be to preinitialize the events array
and return zeroes to the host, but I don't know if that's the right
solution.

I think that's out of spec.

quoted

Another option would be to return 'idx' from update_balloon_stats,
and use that for the size:

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index cd5c54e2003d..bea3cfb88e53 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c

@@ -242,7 +242,7 @@ static inline void update_stat(struct

virtio_balloon *vb, int idx,

 #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)

-static void update_balloon_stats(struct virtio_balloon *vb)
+static unsigned int update_balloon_stats(struct virtio_balloon *vb)
 {
  unsigned long events[NR_VM_EVENT_ITEMS];
  struct sysinfo i;

@@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
  pages_to_bytes(i.totalram));
  update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
  pages_to_bytes(available));
+
+ return idx;
 }

 /*

@@ -294,13 +296,14 @@ static void stats_handle_request(struct

virtio_balloon *vb)
  struct virtqueue *vq;
  struct scatterlist sg;
  unsigned int len;
+ unsigned int num_stats;

- update_balloon_stats(vb);
+ num_stats = update_balloon_stats(vb);

  vq = vb->stats_vq;
  if (!virtqueue_get_buf(vq, &len))
  return;
- sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
  virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
  virtqueue_kick(vq);
 }

     Arnd

Sounds reasonable.

--
MST