[PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Roman Kiryanov]

The vmstate_save_state_v() function does not support
NULL in VMStateDescription::fields and will crash if
one is provided.

This change prevents this situation from happening
by explicitly crashing earlier.

Suggested-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Roman Kiryanov <rkir@google.com>
---
v3:
 - Also added assert to virtio.c to validate
   VirtioDeviceClass instances which bypass
   vmstate_register_with_alias_id.
 - Updated the commit message to "Reviewed-by".
v2:
 - Moved the assert from vmstate_save_state_v to the registration
   path (vmstate_register_with_alias_id) and the qtest validation path
   (vmstate_check) to catch missing fields earlier.
---
 hw/virtio/virtio.c | 6 ++++++
 migration/savevm.c | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0ba734d0bc..e4543de335 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
     /* Devices should either use vmsd or the load/save methods */
     assert(!vdc->vmsd || !vdc->load);
 
+    /*
+     * If a device has vmsd, it either MUST have valid fields
+     * or marked unmigratable.
+     */
+    assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields));
+
     if (vdc->realize != NULL) {
         vdc->realize(dev, &err);
         if (err != NULL) {
diff --git a/migration/savevm.c b/migration/savevm.c
index 197c89e0e6..20c2b99231 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd)
     const VMStateField *field = vmsd->fields;
     const VMStateDescription * const *subsection = vmsd->subsections;
 
+    assert(field || vmsd->unmigratable);
+
     if (field) {
         while (field->name) {
             if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
@@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
     /* If this triggers, alias support can be dropped for the vmsd. */
     assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
 
+    /* If vmsd is migratable it MUST have valid fields. */
+    assert(vmsd->fields || vmsd->unmigratable);
+
     se = g_new0(SaveStateEntry, 1);
     se->version_id = vmsd->version_id;
     se->section_id = savevm_state.global_section_id++;
-- 
2.53.0.851.ga537e3e6e9-goog

Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Fabiano Rosas]

Roman Kiryanov <rkir@google.com> writes:

> The vmstate_save_state_v() function does not support
> NULL in VMStateDescription::fields and will crash if
> one is provided.
>
> This change prevents this situation from happening
> by explicitly crashing earlier.
>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Roman Kiryanov <rkir@google.com>
> ---
> v3:
>  - Also added assert to virtio.c to validate
>    VirtioDeviceClass instances which bypass
>    vmstate_register_with_alias_id.
>  - Updated the commit message to "Reviewed-by".
> v2:
>  - Moved the assert from vmstate_save_state_v to the registration
>    path (vmstate_register_with_alias_id) and the qtest validation path
>    (vmstate_check) to catch missing fields earlier.
> ---
>  hw/virtio/virtio.c | 6 ++++++
>  migration/savevm.c | 5 +++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0ba734d0bc..e4543de335 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
>      /* Devices should either use vmsd or the load/save methods */
>      assert(!vdc->vmsd || !vdc->load);
>  
> +    /*
> +     * If a device has vmsd, it either MUST have valid fields
> +     * or marked unmigratable.
> +     */
> +    assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields));
> +
>      if (vdc->realize != NULL) {
>          vdc->realize(dev, &err);
>          if (err != NULL) {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 197c89e0e6..20c2b99231 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      const VMStateField *field = vmsd->fields;
>      const VMStateDescription * const *subsection = vmsd->subsections;
>  
> +    assert(field || vmsd->unmigratable);
> +
>      if (field) {
>          while (field->name) {
>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>      /* If this triggers, alias support can be dropped for the vmsd. */
>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
>  
> +    /* If vmsd is migratable it MUST have valid fields. */
> +    assert(vmsd->fields || vmsd->unmigratable);
> +
>      se = g_new0(SaveStateEntry, 1);
>      se->version_id = vmsd->version_id;
>      se->section_id = savevm_state.global_section_id++;

Looks like we'll also need to fix some stubs that define empty vmsds:

qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const
VMStateDescription *): Assertion `field || vmsd->unmigratable' failed.

Maybe something like the following. Peter, what do you think?

-- >8 --
From 9bd63109e970ff231eb321e627f622910f9977ca Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Mon, 16 Mar 2026 11:16:22 -0300
Subject: [PATCH] fixup! vmstate: validate VMStateDescription::fields upon
 registration

---
 hw/acpi/acpi-cpu-hotplug-stub.c | 4 +++-
 hw/acpi/acpi-mem-hotplug-stub.c | 4 +++-
 hw/acpi/acpi-pci-hotplug-stub.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index 72c5f05f5c..4332f3fb7d 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -3,7 +3,9 @@
 #include "hw/acpi/cpu.h"
 
 /* Following stubs are all related to ACPI cpu hotplug */
-const VMStateDescription vmstate_cpu_hotplug;
+const VMStateDescription vmstate_cpu_hotplug = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};
 
 void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
                          CPUHotplugState *state, hwaddr base_addr)
diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
index 7ad0fdcdf2..36c12a4e5f 100644
--- a/hw/acpi/acpi-mem-hotplug-stub.c
+++ b/hw/acpi/acpi-mem-hotplug-stub.c
@@ -2,7 +2,9 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "migration/vmstate.h"
 
-const VMStateDescription vmstate_memory_hotplug;
+const VMStateDescription vmstate_memory_hotplug = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};
 
 void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
                               MemHotplugState *state, hwaddr io_base)
diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
index d58ea726a8..3e3a484d3e 100644
--- a/hw/acpi/acpi-pci-hotplug-stub.c
+++ b/hw/acpi/acpi-pci-hotplug-stub.c
@@ -2,7 +2,9 @@
 #include "hw/acpi/pcihp.h"
 #include "migration/vmstate.h"
 
-const VMStateDescription vmstate_acpi_pcihp_pci_status;
+const VMStateDescription vmstate_acpi_pcihp_pci_status = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};
 
 void acpi_pcihp_init(Object *owner, AcpiPciHpState *s,
                      MemoryRegion *address_space_io, uint16_t io_base)
-- 
2.51.0

Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Peter Xu]

On Mon, Mar 16, 2026 at 11:21:31AM -0300, Fabiano Rosas wrote:
> Looks like we'll also need to fix some stubs that define empty vmsds:
> 
> qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const
> VMStateDescription *): Assertion `field || vmsd->unmigratable' failed.
> 
> Maybe something like the following. Peter, what do you think?

Yep, sounds ok.

-- 
Peter Xu

Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Peter Maydell]

On Mon, 16 Mar 2026 at 14:22, Fabiano Rosas <farosas@suse.de> wrote:
>
> Roman Kiryanov <rkir@google.com> writes:
>
> > The vmstate_save_state_v() function does not support
> > NULL in VMStateDescription::fields and will crash if
> > one is provided.
> >
> > This change prevents this situation from happening
> > by explicitly crashing earlier.
> >
> > Suggested-by: Fabiano Rosas <farosas@suse.de>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Roman Kiryanov <rkir@google.com>
> > ---
> > v3:
> >  - Also added assert to virtio.c to validate
> >    VirtioDeviceClass instances which bypass
> >    vmstate_register_with_alias_id.
> >  - Updated the commit message to "Reviewed-by".
> > v2:
> >  - Moved the assert from vmstate_save_state_v to the registration
> >    path (vmstate_register_with_alias_id) and the qtest validation path
> >    (vmstate_check) to catch missing fields earlier.
> > ---
> >  hw/virtio/virtio.c | 6 ++++++
> >  migration/savevm.c | 5 +++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 0ba734d0bc..e4543de335 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> >      /* Devices should either use vmsd or the load/save methods */
> >      assert(!vdc->vmsd || !vdc->load);
> >
> > +    /*
> > +     * If a device has vmsd, it either MUST have valid fields
> > +     * or marked unmigratable.
> > +     */
> > +    assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields));
> > +
> >      if (vdc->realize != NULL) {
> >          vdc->realize(dev, &err);
> >          if (err != NULL) {
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 197c89e0e6..20c2b99231 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd)
> >      const VMStateField *field = vmsd->fields;
> >      const VMStateDescription * const *subsection = vmsd->subsections;
> >
> > +    assert(field || vmsd->unmigratable);
> > +
> >      if (field) {
> >          while (field->name) {
> >              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> > @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
> >      /* If this triggers, alias support can be dropped for the vmsd. */
> >      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
> >
> > +    /* If vmsd is migratable it MUST have valid fields. */
> > +    assert(vmsd->fields || vmsd->unmigratable);
> > +
> >      se = g_new0(SaveStateEntry, 1);
> >      se->version_id = vmsd->version_id;
> >      se->section_id = savevm_state.global_section_id++;
>
> Looks like we'll also need to fix some stubs that define empty vmsds:
>
> qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const
> VMStateDescription *): Assertion `field || vmsd->unmigratable' failed.
>
> Maybe something like the following. Peter, what do you think?

Ah, I was about to report this -- commit 7aa563630b6b0 ("pc: Start
with modern CPU hotplug interface by default") broke the MIPS Malta
board migration, which now segfaults when you "savevm" because
the stub vmstate_cpu_hotplug struct has a NULL fields pointer.

This used to work because the relevant vmstate had a "needed"
function that meant we never used the stub vmstate structs, but
that commit dropped the "needed" functions.

If the change you suggest is the right way to fix this then it
ought to be its own patch with a Fixes: tag pointing at 7aa563630b6b0.

thanks
-- PMM

Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Philippe Mathieu-Daudé]

On 16/3/26 15:21, Fabiano Rosas wrote:

> -- >8 --
>  From 9bd63109e970ff231eb321e627f622910f9977ca Mon Sep 17 00:00:00 2001
> From: Fabiano Rosas <farosas@suse.de>
> Date: Mon, 16 Mar 2026 11:16:22 -0300
> Subject: [PATCH] fixup! vmstate: validate VMStateDescription::fields upon
>   registration
> 
> ---
>   hw/acpi/acpi-cpu-hotplug-stub.c | 4 +++-
>   hw/acpi/acpi-mem-hotplug-stub.c | 4 +++-
>   hw/acpi/acpi-pci-hotplug-stub.c | 4 +++-
>   3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> index 72c5f05f5c..4332f3fb7d 100644
> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> @@ -3,7 +3,9 @@
>   #include "hw/acpi/cpu.h"
>   
>   /* Following stubs are all related to ACPI cpu hotplug */
> -const VMStateDescription vmstate_cpu_hotplug;
> +const VMStateDescription vmstate_cpu_hotplug = {
> +    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
> +};
>   
>   void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>                            CPUHotplugState *state, hwaddr base_addr)
> diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
> index 7ad0fdcdf2..36c12a4e5f 100644
> --- a/hw/acpi/acpi-mem-hotplug-stub.c
> +++ b/hw/acpi/acpi-mem-hotplug-stub.c
> @@ -2,7 +2,9 @@
>   #include "hw/acpi/memory_hotplug.h"
>   #include "migration/vmstate.h"
>   
> -const VMStateDescription vmstate_memory_hotplug;
> +const VMStateDescription vmstate_memory_hotplug = {
> +    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
> +};
>   
>   void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
>                                 MemHotplugState *state, hwaddr io_base)
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> index d58ea726a8..3e3a484d3e 100644
> --- a/hw/acpi/acpi-pci-hotplug-stub.c
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -2,7 +2,9 @@
>   #include "hw/acpi/pcihp.h"
>   #include "migration/vmstate.h"
>   
> -const VMStateDescription vmstate_acpi_pcihp_pci_status;
> +const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> +    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
> +};
>   
>   void acpi_pcihp_init(Object *owner, AcpiPciHpState *s,
>                        MemoryRegion *address_space_io, uint16_t io_base)

Missing:

-- >8 --
diff --git a/hw/display/ramfb-stubs.c b/hw/display/ramfb-stubs.c
index b83551357bb..283af44ecec 100644
--- a/hw/display/ramfb-stubs.c
+++ b/hw/display/ramfb-stubs.c
@@ -2,7 +2,9 @@
  #include "qapi/error.h"
  #include "hw/display/ramfb.h"

-const VMStateDescription ramfb_vmstate = {};
+const VMStateDescription ramfb_vmstate = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};

  void ramfb_display_update(QemuConsole *con, RAMFBState *s)
  {
---

Otherwise LGTM (agreeing with Peter to post separately as
a preliminary Fixes patch).

Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Fabiano Rosas]

Roman Kiryanov <rkir@google.com> writes:

> The vmstate_save_state_v() function does not support
> NULL in VMStateDescription::fields and will crash if
> one is provided.
>
> This change prevents this situation from happening
> by explicitly crashing earlier.
>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Roman Kiryanov <rkir@google.com>
> ---
> v3:
>  - Also added assert to virtio.c to validate
>    VirtioDeviceClass instances which bypass
>    vmstate_register_with_alias_id.
>  - Updated the commit message to "Reviewed-by".
> v2:
>  - Moved the assert from vmstate_save_state_v to the registration
>    path (vmstate_register_with_alias_id) and the qtest validation path
>    (vmstate_check) to catch missing fields earlier.
> ---
>  hw/virtio/virtio.c | 6 ++++++
>  migration/savevm.c | 5 +++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0ba734d0bc..e4543de335 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
>      /* Devices should either use vmsd or the load/save methods */
>      assert(!vdc->vmsd || !vdc->load);
>  
> +    /*
> +     * If a device has vmsd, it either MUST have valid fields
> +     * or marked unmigratable.
> +     */
> +    assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields));
> +
>      if (vdc->realize != NULL) {
>          vdc->realize(dev, &err);
>          if (err != NULL) {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 197c89e0e6..20c2b99231 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      const VMStateField *field = vmsd->fields;
>      const VMStateDescription * const *subsection = vmsd->subsections;
>  
> +    assert(field || vmsd->unmigratable);
> +
>      if (field) {
>          while (field->name) {
>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>      /* If this triggers, alias support can be dropped for the vmsd. */
>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
>  
> +    /* If vmsd is migratable it MUST have valid fields. */
> +    assert(vmsd->fields || vmsd->unmigratable);
> +
>      se = g_new0(SaveStateEntry, 1);
>      se->version_id = vmsd->version_id;
>      se->section_id = savevm_state.global_section_id++;

Hi, this patch missed the train. It's crashing the
./scripts/device-crash-test

The vmstate_virtio_snd_device needs to be fixed first.

---
Some thoughts, no action needed:

I'm cautions after the issue with the stubs. We should expect that a
stub with all fields zeroed could still reach the vmstate code. Maybe
it'll be fine with the (eventual) fix for mips, but we need to test it
thoroughly.

There is also the -dump-vmstate command line option, which can certainly
process vmsds with incorrect fields as it takes them directly from the
device class. It doesn't crash, but prints stuff like:

"Fields": [{
    "field": "cpuhp_state",
    "version_id": 1,
    "field_exists": false,
    "size": 304,
    "Description": {
        "name": "(null)",  <---
        "version_id": 0,
        "minimum_version_id": 0
    }}]

Perhaps it should even do a validation pass and warn or add a comment to
the output.