On Fri, Jun 27, 2014 at 09:34:38AM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>
> 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.
I think you mean wce.
>
> 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 <dgilb...@redhat.com>
Looks good overall, but I am having thoughts about the
padding with 0xff.
We previously didn't do this (before virtio: validate config_len on
load) so it seems safest (at least for 2.1) not to do it now either.
> ---
> 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.
> + */
> +
Drop extra line please.
> + 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);
So I would say handle config_len < vdev->config_len and config_len ==
vdev->config_len the same:
qemu_get_buffer(f, vdev->config, MIN(config_len, vdev->config_len));
and then skip the remainder if any
while (config_len > vdev->config_len) {
qemu_get_byte(f);
config_len--;
}
>
> --
> 1.9.3
