Re: [PATCH v2] vmstate: fix subsection load name check

[Fabiano Rosas <farosas@suse.de>]

Alexandr Moshkov <dtalexundeer@yandex-team.ru> writes:

> When loading a subset, its name is checked for the parent prefix. The
> following bug may occur here:
>
> Let's say there is a vmstate named "virtio-blk", it has a subsection
> named "virtio-blk/subsection", and it also has another vmstate named
> "virtio" in the fields.
> Then, during the migration, when trying to load this subsection for
> "virtio", the prefix condition will pass for "virtio-blk/subsection" and
> then the migration will break, because this vmstate does not have such a
> subsection.
>
> In other words, if a field inside vmstate1 is set via vmstate2 with a
> name that is a prefix of the parent vmstate, then the field can "steal"
> a subsection belonging to the parent state.
>
> Looks like it happens because migration stream for "virtio-blk" looks
> like this:
>
>  [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>
> "virtio-blk" contains "virtio" field, so migration stream is:
>
>  [virtio-blk header] [virtio header] [virtio fields] [virtio
> subsections] [virtio-blk subsections]
>
> And when we load the subsections of the "virtio" device,
> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
> this is his subsection. This is where we encounter an error.
>
> Thus, the error occurs due to the fact that vmsd does not know how many
> subsections it has when loading (this does not appear anywhere in the
> migration stream), so it tries to load all the appropriate ones by
> names.
>
> Fix it by checking `/` at the end of idstr.
>
> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> ---
>  migration/vmstate.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 4d28364f7b..187f3861f2 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -613,7 +613,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>  
>      while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
>          char idstr[256], *idstr_ret;
> -        int ret;
> +        int ret, vmsd_name_len;
>          uint8_t version_id, len, size;
>          const VMStateDescription *sub_vmsd;
>  
> @@ -631,7 +631,10 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          memcpy(idstr, idstr_ret, size);
>          idstr[size] = 0;
>  
> -        if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
> +        vmsd_name_len = strlen(vmsd->name);
> +        if (strncmp(vmsd->name, idstr, vmsd_name_len) != 0 ||
> +            /* to avoid taking parent subsection here */
> +            idstr[vmsd_name_len] != '/') {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(prefix)");
>              /* it doesn't have a valid subsection name */
>              return 0;

Hi, this breaks migration for s390x and ppc64:

qemu-system-s390x: Missing section footer for qemu-s390-flic

qemu-system-ppc64: Missing section footer for spapr_iommu