Fix piix4_pm savevm buffer overflow.

Submitted by Gerd Hoffmann on Nov. 8, 2012, 6:21 p.m.

Details

Message ID 1352398871-21705-1-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann Nov. 8, 2012, 6:21 p.m.
vmstate will read/write 8 bytes for the gpe bits, whereas only two bytes
are allocated (and used).  So make sure we allocate enougth to not
overrun the buffer on savevm and loadvm.

Fixing vmstate would probably be better but that has the unpleasent side
effect of breaking migration.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/acpi.c |   10 ++++++++--
 savevm.c  |    6 ++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Nov. 9, 2012, 8:14 a.m.
Il 08/11/2012 19:21, Gerd Hoffmann ha scritto:
> vmstate will read/write 8 bytes for the gpe bits, whereas only two bytes
> are allocated (and used).  So make sure we allocate enougth to not
> overrun the buffer on savevm and loadvm.
> 
> Fixing vmstate would probably be better but that has the unpleasent side
> effect of breaking migration.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/acpi.c |   10 ++++++++--
>  savevm.c  |    6 ++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi.c b/hw/acpi.c
> index f4aca49..6ed76ff 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -406,9 +406,15 @@ void acpi_pm1_cnt_reset(ACPIREGS *ar)
>  /* ACPI GPE */
>  void acpi_gpe_init(ACPIREGS *ar, uint8_t len)
>  {
> +    /*
> +     * Hack alert: Although we are using only two bytes (GPE_LEN / 2)
> +     * for each of "sts" and "en" we have to allocate more because
> +     * VMSTATE_GPE_ARRAY() writes 8 bytes (GPE_LEN * sizeof(uint16_t))
> +     * to the vmstate stream.
> +     */
>      ar->gpe.len = len;
> -    ar->gpe.sts = g_malloc0(len / 2);
> -    ar->gpe.en = g_malloc0(len / 2);
> +    ar->gpe.sts = g_malloc0(len * 2);
> +    ar->gpe.en = g_malloc0(len * 2);

If the other 6 bytes are really unused, please use VMSTATE_UNUSED or
VMSTATE_UNUSED_V to skip them in the vmstate stream.

(Also, debug stuff below).

Paolo

>  }
>  
>  void acpi_gpe_blk(ACPIREGS *ar, uint32_t blk)
> diff --git a/savevm.c b/savevm.c
> index 5d04d59..be0257e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1511,6 +1511,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>  {
>      VMStateField *field = vmsd->fields;
>  
> +    fprintf(stderr, "%s: %s\n", __func__, vmsd->name);
> +
>      if (vmsd->pre_save) {
>          vmsd->pre_save(opaque);
>      }
> @@ -1521,6 +1523,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>              int i, n_elems = 1;
>              int size = field->size;
>  
> +            fprintf(stderr, "%s:   %s\n", __func__, field->name);
> +
>              if (field->flags & VMS_VBUFFER) {
>                  size = *(int32_t *)(opaque+field->size_offset);
>                  if (field->flags & VMS_MULTIPLY) {
> @@ -1550,6 +1554,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                  if (field->flags & VMS_STRUCT) {
>                      vmstate_save_state(f, field->vmsd, addr);
>                  } else {
> +                    fprintf(stderr, "%s:     a %p s %d %d/%d\n", __func__,
> +                            addr, size, i, n_elems);
>                      field->info->put(f, addr, size);
>                  }
>              }
>

Patch hide | download patch | download mbox

diff --git a/hw/acpi.c b/hw/acpi.c
index f4aca49..6ed76ff 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -406,9 +406,15 @@  void acpi_pm1_cnt_reset(ACPIREGS *ar)
 /* ACPI GPE */
 void acpi_gpe_init(ACPIREGS *ar, uint8_t len)
 {
+    /*
+     * Hack alert: Although we are using only two bytes (GPE_LEN / 2)
+     * for each of "sts" and "en" we have to allocate more because
+     * VMSTATE_GPE_ARRAY() writes 8 bytes (GPE_LEN * sizeof(uint16_t))
+     * to the vmstate stream.
+     */
     ar->gpe.len = len;
-    ar->gpe.sts = g_malloc0(len / 2);
-    ar->gpe.en = g_malloc0(len / 2);
+    ar->gpe.sts = g_malloc0(len * 2);
+    ar->gpe.en = g_malloc0(len * 2);
 }
 
 void acpi_gpe_blk(ACPIREGS *ar, uint32_t blk)
diff --git a/savevm.c b/savevm.c
index 5d04d59..be0257e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1511,6 +1511,8 @@  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
 {
     VMStateField *field = vmsd->fields;
 
+    fprintf(stderr, "%s: %s\n", __func__, vmsd->name);
+
     if (vmsd->pre_save) {
         vmsd->pre_save(opaque);
     }
@@ -1521,6 +1523,8 @@  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
             int i, n_elems = 1;
             int size = field->size;
 
+            fprintf(stderr, "%s:   %s\n", __func__, field->name);
+
             if (field->flags & VMS_VBUFFER) {
                 size = *(int32_t *)(opaque+field->size_offset);
                 if (field->flags & VMS_MULTIPLY) {
@@ -1550,6 +1554,8 @@  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                 if (field->flags & VMS_STRUCT) {
                     vmstate_save_state(f, field->vmsd, addr);
                 } else {
+                    fprintf(stderr, "%s:     a %p s %d %d/%d\n", __func__,
+                            addr, size, i, n_elems);
                     field->info->put(f, addr, size);
                 }
             }