Re: [PATCH] virtio_balloon: prevent uninitialized variable use
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2017-03-24 20:59:47
Also in:
lkml
On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
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? 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 hunk ↗ jump to hunk
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(structvirtio_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(structvirtio_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