[Qemu-devel] [PATCH v2] savevm: fix corruption in vmstate_subsection_load().

[Qemu-devel] [PATCH v2] savevm: fix corruption in vmstate_subsection_load().

[Yoshiaki Tamura]

Although it's rare to happen in live migration, when the head of a
byte stream contains 0x05 which is the marker of subsection, the
loader gets corrupted because vmstate_subsection_load() continues even
the device doesn't require it.  This patch adds a checker whether
subsection is needed, and skips following routines if not needed.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 savevm.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index fcd8db4..3be875d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1639,6 +1639,12 @@ static const VMStateDescription *vmstate_get_subsection(const VMStateSubsection
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque)
 {
+    const VMStateSubsection *sub = vmsd->subsections;
+
+    if (!sub || !sub->needed) {
+        return 0;
+    }
+
     while (qemu_peek_byte(f) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         int ret;
@@ -1651,10 +1657,11 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         idstr[len] = 0;
         version_id = qemu_get_be32(f);
 
-        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
+        sub_vmsd = vmstate_get_subsection(sub, idstr);
         if (sub_vmsd == NULL) {
             return -ENOENT;
         }
+        assert(!sub_vmsd->subsections);
         ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
         if (ret) {
             return ret;
@@ -1678,6 +1685,7 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
             qemu_put_byte(f, len);
             qemu_put_buffer(f, (uint8_t *)vmsd->name, len);
             qemu_put_be32(f, vmsd->version_id);
+            assert(!vmsd->subsections);
             vmstate_save_state(f, vmsd, opaque);
         }
         sub++;
-- 
1.7.1.2

[Qemu-devel] Re: [PATCH v2] savevm: fix corruption in vmstate_subsection_load().

[Paolo Bonzini]

On 01/24/2011 10:30 AM, Yoshiaki Tamura wrote:
> Although it's rare to happen in live migration, when the head of a
> byte stream contains 0x05 which is the marker of subsection, the
> loader gets corrupted because vmstate_subsection_load() continues even
> the device doesn't require it.  This patch adds a checker whether
> subsection is needed, and skips following routines if not needed.
>
> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>

VMS_STRUCT is a can of worms, since almost all uses of subsections _are_ 
VMS_STRUCTs.  So almost all uses of subsections have ambiguous parsing.  :/

So, for now,

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v2] savevm: fix corruption in vmstate_subsection_load().

[Kevin Wolf]

Am 24.01.2011 10:30, schrieb Yoshiaki Tamura:
> Although it's rare to happen in live migration, when the head of a
> byte stream contains 0x05 which is the marker of subsection, the
> loader gets corrupted because vmstate_subsection_load() continues even
> the device doesn't require it.  This patch adds a checker whether
> subsection is needed, and skips following routines if not needed.
> 
> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
> ---
>  savevm.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index fcd8db4..3be875d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1639,6 +1639,12 @@ static const VMStateDescription *vmstate_get_subsection(const VMStateSubsection
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>                                     void *opaque)
>  {
> +    const VMStateSubsection *sub = vmsd->subsections;
> +
> +    if (!sub || !sub->needed) {
> +        return 0;
> +    }

This means that subsections are silently ignored if the old version
didn't have any subsection for the device. Wasn't it a fundamental point
of the design of subsections that this would fail?

Noticed this when trying to use subsections to fix up IDE, and new ->
old migration after an I/O error only correctly fails when I revert this
patch.

Kevin

Re: [Qemu-devel] [PATCH v2] savevm: fix corruption in vmstate_subsection_load().

[Paolo Bonzini]

On 06/09/2011 02:48 PM, Kevin Wolf wrote:
> This means that subsections are silently ignored if the old version
> didn't have any subsection for the device. Wasn't it a fundamental point
> of the design of subsections that this would fail?

Unfortunately you cannot have it both ways because the migration format 
is ambiguous.

> Noticed this when trying to use subsections to fix up IDE, and new ->
> old migration after an I/O error only correctly fails when I revert this
> patch.

I would have expected migration to fail anyway because the next 
instances are not parsed correctly.

Paolo