From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50679 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PfqUK-00030g-8r for qemu-devel@nongnu.org; Thu, 20 Jan 2011 04:07:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PfqUG-0005U3-1B for qemu-devel@nongnu.org; Thu, 20 Jan 2011 04:07:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:25803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PfqUF-0005Tw-Qc for qemu-devel@nongnu.org; Thu, 20 Jan 2011 04:07:23 -0500 Message-ID: <4D37FB2F.1050803@redhat.com> Date: Thu, 20 Jan 2011 10:06:55 +0100 From: Paolo Bonzini MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] savevm: fix corruption in vmstate_subsection_load(). References: <1292317658-15798-1-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <4D37F20F.5040008@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yoshiaki Tamura Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, quintela@redhat.com On 01/20/2011 09:57 AM, Yoshiaki Tamura wrote: > 2011/1/20 Paolo Bonzini: >> 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