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

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

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@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.

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 <dgilbert@redhat.com>
---
 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);
 
-- 
1.9.3

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" <dgilbert@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.

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 <dgilbert@redhat.com>
> ---
>  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);
>
>

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

[Michael S. Tsirkin]

On Fri, Jun 27, 2014 at 09:34:38AM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@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 <dgilbert@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

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

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Fri, Jun 27, 2014 at 09:34:38AM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@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.

Oops - yes.

> > 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 <dgilbert@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.

Who allocates that memory? If it's known to be a value then I agree; however
if it's uninitialised then I think it's best to pick a value rather than
have behaviour that depends on random junk in the memory.

> > ---
> >  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--;
> 	}

That probably still needs that MIN splitting out (int32_t vs size_t); but
other than that I guess I can redo that.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Michael S. Tsirkin]

On Fri, Jun 27, 2014 at 03:42:10PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Fri, Jun 27, 2014 at 09:34:38AM +0100, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@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.
> 
> Oops - yes.
> 
> > > 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 <dgilbert@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.
> 
> Who allocates that memory? If it's known to be a value then I agree; however
> if it's uninitialised then I think it's best to pick a value rather than
> have behaviour that depends on random junk in the memory.

It's initialized: e.g. for net it includes the mac,
for block the wce value.

> > > ---
> > >  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--;
> > 	}
> 
> That probably still needs that MIN splitting out (int32_t vs size_t); but
> other than that I guess I can redo that.
> 
> Dave
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Fri, Jun 27, 2014 at 03:42:10PM +0100, Dr. David Alan Gilbert wrote:
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Fri, Jun 27, 2014 at 09:34:38AM +0100, Dr. David Alan Gilbert (git) wrote:
> > > > From: "Dr. David Alan Gilbert" <dgilbert@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.
> > 
> > Oops - yes.
> > 
> > > > 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 <dgilbert@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.
> > 
> > Who allocates that memory? If it's known to be a value then I agree; however
> > if it's uninitialised then I think it's best to pick a value rather than
> > have behaviour that depends on random junk in the memory.
> 
> It's initialized: e.g. for net it includes the mac,
> for block the wce value.

OK, I've just posted a v2 that is the simplified code you suggested.
(Not had quite as much testing as my previous version yet).

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK