[Qemu-devel] [PATCH] hw/virtio/virtio-balloon: zero-initialize the virtio_balloon_config struct

[Qemu-devel] [PATCH] hw/virtio/virtio-balloon: zero-initialize the virtio_balloon_config struct

[Peter Maydell]

In virtio_balloon_get_config() we initialize a struct virtio_balloon_config
which we then copy to guest memory. However, the local variable is not
zero initialized. This works OK at the moment because we initialize
all the fields in it; however an upcoming kernel header change will
add some new fields. If we don't zero out the whole struct then we
will start leaking a small amount of the contents of QEMU's stack
to the guest as soon as we update linux-headers/ to a set of headers
that includes the new fields.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
It looks like none of the other virtio devices have this bug.
Tested with "make check" only.
As the commit message notes, must go in before our next headers update.
---
 hw/virtio/virtio-balloon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1728e4f83af..a12677d4d5b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -311,7 +311,7 @@ out:
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
-    struct virtio_balloon_config config;
+    struct virtio_balloon_config config = {};
 
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
-- 
2.20.1

Re: [Qemu-devel] [PATCH] hw/virtio/virtio-balloon: zero-initialize the virtio_balloon_config struct

[Michael S. Tsirkin]

On Fri, Jan 18, 2019 at 06:36:03PM +0000, Peter Maydell wrote:
> In virtio_balloon_get_config() we initialize a struct virtio_balloon_config
> which we then copy to guest memory. However, the local variable is not
> zero initialized. This works OK at the moment because we initialize
> all the fields in it; however an upcoming kernel header change will
> add some new fields. If we don't zero out the whole struct then we
> will start leaking a small amount of the contents of QEMU's stack
> to the guest as soon as we update linux-headers/ to a set of headers
> that includes the new fields.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> It looks like none of the other virtio devices have this bug.
> Tested with "make check" only.
> As the commit message notes, must go in before our next headers update.

Want me to merge it? Or you can pick it up directly if you like
to make sure that happens.

> ---
>  hw/virtio/virtio-balloon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 1728e4f83af..a12677d4d5b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -311,7 +311,7 @@ out:
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> -    struct virtio_balloon_config config;
> +    struct virtio_balloon_config config = {};
>  
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
> -- 
> 2.20.1

Re: [Qemu-devel] [PATCH] hw/virtio/virtio-balloon: zero-initialize the virtio_balloon_config struct

[Philippe Mathieu-Daudé]

On 1/18/19 7:36 PM, Peter Maydell wrote:
> In virtio_balloon_get_config() we initialize a struct virtio_balloon_config
> which we then copy to guest memory. However, the local variable is not
> zero initialized. This works OK at the moment because we initialize
> all the fields in it; however an upcoming kernel header change will
> add some new fields. If we don't zero out the whole struct then we
> will start leaking a small amount of the contents of QEMU's stack
> to the guest as soon as we update linux-headers/ to a set of headers
> that includes the new fields.

Is it worth Cc'ing qemu-stable@?

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> It looks like none of the other virtio devices have this bug.
> Tested with "make check" only.
> As the commit message notes, must go in before our next headers update.
> ---
>  hw/virtio/virtio-balloon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 1728e4f83af..a12677d4d5b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -311,7 +311,7 @@ out:
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> -    struct virtio_balloon_config config;
> +    struct virtio_balloon_config config = {};
>  
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
> 

Re: [Qemu-devel] [PATCH] hw/virtio/virtio-balloon: zero-initialize the virtio_balloon_config struct

[Peter Maydell]

On Mon, 21 Jan 2019 at 10:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 1/18/19 7:36 PM, Peter Maydell wrote:
> > In virtio_balloon_get_config() we initialize a struct virtio_balloon_config
> > which we then copy to guest memory. However, the local variable is not
> > zero initialized. This works OK at the moment because we initialize
> > all the fields in it; however an upcoming kernel header change will
> > add some new fields. If we don't zero out the whole struct then we
> > will start leaking a small amount of the contents of QEMU's stack
> > to the guest as soon as we update linux-headers/ to a set of headers
> > that includes the new fields.
>
> Is it worth Cc'ing qemu-stable@?

Good idea, that will avoid possible future problems if we backport
a headers change to stable.

I'll apply this to master directly.

thanks
-- PMM