[PATCH v2] virtio_balloon: don't push uninitialized buffers to stats virtqueue

[PATCH v2] virtio_balloon: don't push uninitialized buffers to stats virtqueue

[Ladi Prosek]

When init_vqs runs, virtio_balloon.stats is either uninitialized or
contains stale values. The host updates its state with garbage data
because it has no way of knowing that this is just a marker buffer
used for signaling.

This patch updates the stats before pushing the initial buffer.
An assertion is also added to update_balloon_stats to make sure
that all stats fields are initialized.

Alternative fixes:
* Push an empty buffer in init_vqs. Not easily done with the current
  virtio implementation and violates the spec "Driver MUST supply the
  same subset of statistics in all buffers submitted to the statsq".
* Push a buffer with invalid tags in init_vqs. Violates the same
  spec clause, plus "invalid tag" is not really defined.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---

v1->v2:

* Call update_balloon_stats instead of filling the buffer with
  invalid tags.
* Add BUG_ON to update_balloon_stats to formalize the invariant that
  all slots in the buffer must be initialized.


Also, I just noticed this paragraph in the spec:

"When using the legacy interface, the device SHOULD ignore all values in
the first buffer in the statsq supplied by the driver after device
initialization. Note: Historically, drivers supplied an uninitialized
buffer in the first buffer."

So someone knew about this bug but didn't fix the Linux driver and also
didn't implement the SHOULD in QEMU. Makes me wonder if I'm missing
something here.

Thanks!


 drivers/virtio/virtio_balloon.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e11915..42dc35f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -266,6 +266,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));
+
+	BUG_ON(idx < VIRTIO_BALLOON_S_NR);
 }
 
 /*
@@ -429,6 +431,8 @@ static int init_vqs(struct virtio_balloon *vb)
 		 * Prime this virtqueue with one buffer so the hypervisor can
 		 * use it to signal us later (it can't be broken yet!).
 		 */
+		update_balloon_stats(vb);
+
 		sg_init_one(&sg, vb->stats, sizeof vb->stats);
 		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
 		    < 0)
-- 
2.7.4

Re: [PATCH v2] virtio_balloon: don't push uninitialized buffers to stats virtqueue

[Michael S. Tsirkin]

On Thu, Mar 23, 2017 at 08:04:18AM +0100, Ladi Prosek wrote:
> When init_vqs runs, virtio_balloon.stats is either uninitialized or
> contains stale values. The host updates its state with garbage data
> because it has no way of knowing that this is just a marker buffer
> used for signaling.
> 
> This patch updates the stats before pushing the initial buffer.
> An assertion is also added to update_balloon_stats to make sure
> that all stats fields are initialized.
> 
> Alternative fixes:
> * Push an empty buffer in init_vqs. Not easily done with the current
>   virtio implementation and violates the spec "Driver MUST supply the
>   same subset of statistics in all buffers submitted to the statsq".
> * Push a buffer with invalid tags in init_vqs. Violates the same
>   spec clause, plus "invalid tag" is not really defined.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
> 
> v1->v2:
> 
> * Call update_balloon_stats instead of filling the buffer with
>   invalid tags.
> * Add BUG_ON to update_balloon_stats to formalize the invariant that
>   all slots in the buffer must be initialized.
> 
> 
> Also, I just noticed this paragraph in the spec:
> 
> "When using the legacy interface, the device SHOULD ignore all values in
> the first buffer in the statsq supplied by the driver after device
> initialization. Note: Historically, drivers supplied an uninitialized
> buffer in the first buffer."
> 
> So someone knew about this bug but didn't fix the Linux driver and also
> didn't implement the SHOULD in QEMU. Makes me wonder if I'm missing
> something here.
> 
> Thanks!

I would say let's fix QEMU as well.

> 
>  drivers/virtio/virtio_balloon.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e11915..42dc35f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -266,6 +266,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));
> +
> +	BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>  }
>  
>  /*
> @@ -429,6 +431,8 @@ static int init_vqs(struct virtio_balloon *vb)
>  		 * Prime this virtqueue with one buffer so the hypervisor can
>  		 * use it to signal us later (it can't be broken yet!).
>  		 */
> +		update_balloon_stats(vb);
> +
>  		sg_init_one(&sg, vb->stats, sizeof vb->stats);
>  		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
>  		    < 0)
> -- 
> 2.7.4