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

[Paolo Bonzini]

On 01/20/2011 09:57 AM, Yoshiaki Tamura wrote:
> 2011/1/20 Paolo Bonzini<address@hidden>:
> > On 12/14/2010 10:07 AM, Yoshiaki Tamura wrote:
> > > Although it's rare to happen in live migration, when the head of a
> > > byte stream contains 0x05
> >
> > IIUC, this happens if you have VMS_STRUCT and the field after the VMS_STRUCT
> > starts with 0x5.
> >
> > I think you should also add this in vmstate_subsection_load:
> >
> >     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);
> >
> > and this in vmstate_load_state:
> >
> >     if (field->flags&  VMS_STRUCT) {
> > +       assert (!vmsd->subsections);
> >         ret = vmstate_load_state(f, field->vmsd, addr,
> >                                  field->vmsd->version_id);
> >     }
>
> Hi Paolo,
>
> You mean, having subsection nested and under VMS_STRUCT are
> violations?

I believe so, because the protocol doesn't allow you to distinguish:

- in the case of nested subsections, whether 2 consecutive subsections 
are siblings, or the second is nested into the first.  In fact, your 
patch also fixes a latent bug in case a device supports more than one 
subsection, and both are present in the data stream.  When 
vmstate_subsection_load is called for the first subsection, it would see 
a 0x5 byte (the beginning of the second subsection), eat it and then 
fail with ENOENT.  The second subsection would then fail to load.

- in the case of VMS_STRUCT, whether a 0x5 byte after the VMS_STRUCT is 
a subsection or part of the parent data stream.  This is, I believe, the 
source of your bug.

I don't think it is possible to fix these problems in the file format 
while preserving backwards compatibility with pre-subsection QEMU (which 
was a fundamental requirement of subsections).  So, I think your patch 
is correct and fixes the practical bugs.  However, we can be even 
stronger and assert that the problematic vmstate descriptions are not used.

Even better, asserts matching the ones above could be added to 
vmstate_subsection_save and vmstate_save_state, as well.

Paolo