Re: [Qemu-devel] [PATCH] Allow mismatched virtio config-len

[Paolo Bonzini]

Il 27/06/2014 10:34, Dr. David Alan Gilbert (git) ha scritto:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Commit 'virtio: validate config_len on load' restricted config_len
> loaded from the wire to match the config_len that the device had.
>
> Unfortunately, there are cases where this isn't true, the one
> we found it on was the wqe addition in virtio-blk.

Indeed, the alternative here is to break migration.

As a follow up, it would be nice to let the bus detect whether the 
config_len change is valid or not.

For virtio-mmio and s390, mst said that config length must always match 
(luckily, these machines aren't versioned so they are not affected by 
the wce change).

For virtio-pci, it is okay as long as the old_length + 
VIRTIO_PCI_REGION_SIZE(vdev) and new_length + 
VIRTIO_PCI_REGION_SIZE(vdev) do not cross a power of two.

Paolo

> Allow mismatched config-lengths:
>    *) If the version on the wire is shorter then ensure that the
>       remainder is 0xff filled (as virtio_config_read does on
>       out of range reads)
>    *) If the version on the wire is longer, load what we have space
>       for and skip the rest.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  hw/virtio/virtio.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a3082d5..2b11142 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -927,11 +927,33 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      }
>      config_len = qemu_get_be32(f);
>      if (config_len != vdev->config_len) {
> -        error_report("Unexpected config length 0x%x. Expected 0x%zx",
> -                     config_len, vdev->config_len);
> -        return -1;
> +        /*
> +         * Unfortunately the reality is that there are cases where we
> +         * see mismatched config lengths, so we have to deal with them
> +         * rather than rejecting them.
> +         */
> +
> +        if (config_len < vdev->config_len) {
> +            /* This is normal in some devices when they add a new option */
> +            memset(vdev->config, 0xff, vdev->config_len);
> +            qemu_get_buffer(f, vdev->config, config_len);
> +        } else {
> +            int32_t diff;
> +            /* config_len > vdev->config_len
> +             * This is rarer, but is here to allow us to fix the case above
> +             */
> +            qemu_get_buffer(f, vdev->config, vdev->config_len);
> +            /*
> +             * Even though we expect the diff to be small, we can't use
> +             * qemu_file_skip because it's not safe for a large skip.
> +             */
> +            for (diff = config_len - vdev->config_len; diff > 0; diff--) {
> +                qemu_get_byte(f);
> +            }
> +        }
> +    } else {
> +        qemu_get_buffer(f, vdev->config, vdev->config_len);
>      }
> -    qemu_get_buffer(f, vdev->config, vdev->config_len);
>
>      num = qemu_get_be32(f);