[PATCH] virtio: fail early on bad config_len in migration

[PATCH] virtio: fail early on bad config_len in migration

[Michael S. Tsirkin]

virtio_load() attempts to load config_len bytes from the migration
stream. If that's huge (e.g. 4g) this will uselessly spin
beyond the end of the stream for seconds. Not nice.
Check qemu_file_get_error() and bail out early, instead.

Also note that config_len is int32_t but is coerced to unsigned when
used. Switch it to uint32_t to make this clearer.

Fixes: 2f5732e964 ("Allow mismatched virtio config-len")
Cc: Dr. David Alan Gilbert <dave@treblig.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3891
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8add4d5d99..72d828d33f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3516,7 +3516,7 @@ int coroutine_mixed_fn
 virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 {
     int i, ret;
-    int32_t config_len;
+    uint32_t config_len;
     uint32_t num;
     uint32_t features;
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
@@ -3564,6 +3564,9 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     qemu_get_buffer(f, vdev->config, MIN(config_len, vdev->config_len));
 
     while (config_len > vdev->config_len) {
+        if (qemu_file_get_error(f)) {
+            return -1;
+        }
         qemu_get_byte(f);
         config_len--;
     }
-- 
MST

Re: [PATCH] virtio: fail early on bad config_len in migration

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> virtio_load() attempts to load config_len bytes from the migration
> stream. If that's huge (e.g. 4g) this will uselessly spin
> beyond the end of the stream for seconds. Not nice.
> Check qemu_file_get_error() and bail out early, instead.
> 
> Also note that config_len is int32_t but is coerced to unsigned when
> used. Switch it to uint32_t to make this clearer.

That in a way feels like the more important fix (I hate to
think what that MIN(int32_t, size_t) does.

> Fixes: 2f5732e964 ("Allow mismatched virtio config-len")
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3891
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  hw/virtio/virtio.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 8add4d5d99..72d828d33f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3516,7 +3516,7 @@ int coroutine_mixed_fn
>  virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>  {
>      int i, ret;
> -    int32_t config_len;
> +    uint32_t config_len;
>      uint32_t num;
>      uint32_t features;
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> @@ -3564,6 +3564,9 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>      qemu_get_buffer(f, vdev->config, MIN(config_len, vdev->config_len));
>  
>      while (config_len > vdev->config_len) {
> +        if (qemu_file_get_error(f)) {
> +            return -1;
> +        }
>          qemu_get_byte(f);
>          config_len--;
>      }
> -- 
> MST
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/