* [Qemu-devel] [PATCH] Allow mismatched virtio config-len
@ 2014-06-27 8:34 Dr. David Alan Gilbert (git)
2014-06-27 11:29 ` Paolo Bonzini
2014-06-27 14:32 ` Michael S. Tsirkin
0 siblings, 2 replies; 6+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2014-06-27 8:34 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, quintela, mst
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow mismatched virtio config-len
2014-06-27 8:34 [Qemu-devel] [PATCH] Allow mismatched virtio config-len Dr. David Alan Gilbert (git)
@ 2014-06-27 11:29 ` Paolo Bonzini
2014-06-27 14:32 ` Michael S. Tsirkin
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-06-27 11:29 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel; +Cc: mst, quintela
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);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow mismatched virtio config-len
2014-06-27 8:34 [Qemu-devel] [PATCH] Allow mismatched virtio config-len Dr. David Alan Gilbert (git)
2014-06-27 11:29 ` Paolo Bonzini
@ 2014-06-27 14:32 ` Michael S. Tsirkin
2014-06-27 14:42 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-06-27 14:32 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: pbonzini, qemu-devel, quintela
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow mismatched virtio config-len
2014-06-27 14:32 ` Michael S. Tsirkin
@ 2014-06-27 14:42 ` Dr. David Alan Gilbert
2014-06-27 15:04 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2014-06-27 14:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: pbonzini, qemu-devel, quintela
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow mismatched virtio config-len
2014-06-27 14:42 ` Dr. David Alan Gilbert
@ 2014-06-27 15:04 ` Michael S. Tsirkin
2014-06-27 19:04 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-06-27 15:04 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: pbonzini, qemu-devel, quintela
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow mismatched virtio config-len
2014-06-27 15:04 ` Michael S. Tsirkin
@ 2014-06-27 19:04 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2014-06-27 19:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: pbonzini, qemu-devel, quintela
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-27 19:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-27 8:34 [Qemu-devel] [PATCH] Allow mismatched virtio config-len Dr. David Alan Gilbert (git)
2014-06-27 11:29 ` Paolo Bonzini
2014-06-27 14:32 ` Michael S. Tsirkin
2014-06-27 14:42 ` Dr. David Alan Gilbert
2014-06-27 15:04 ` Michael S. Tsirkin
2014-06-27 19:04 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.